-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(HMS-2785): pass otel spans properly #711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, let’s merge my PR first where I want to modify the contextLogger
function in worker package to pass trace_id into logs so you can continue on this.
pkg/worker/job.go
Outdated
accountId := job.AccountID | ||
id := job.Identity | ||
logger := zerolog.Ctx(ctx).With(). | ||
Str("trace_id", span.SpanContext().TraceID().String()). | ||
Str("job_id", job.ID.String()). | ||
Int64("account_id", accountId). | ||
Str("account_number", id.Identity.AccountNumber). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this makes a lot of sense. In my patch, I put trace_id at reservation level but I think this is much better place. I will move it here but I will not add any otel related stuff and leave this to you so you can further develop this.
Great idea, in the end, we will have one trace (aka transaction in GlitchTip) and spans for api pod then one big span for the worker pod and additional spans for individual functions.
fb2173d
to
d48b974
Compare
d48b974
to
d963b9b
Compare
Otel needs a root spans, if that is not passed from external service, we should start one ourselves, instead of starting span once we decide there is one needed internally. Also jobs and kafka shall retrieve these spans and continue, not start new spans.
d963b9b
to
30ebff0
Compare
/retest |
/retest |
/retest |
"event_type", "availability_status", | ||
), | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming no changes were made here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the one additional header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so much better than what I came up with. Great work.
Image problem again. |
if config.Telemetry.Enabled { | ||
var traceHeaders propagation.MapCarrier = make(map[string]string) | ||
otel.GetTextMapPropagator().Inject(ctx, traceHeaders) | ||
for name, value := range traceHeaders { | ||
headers = append(headers, GenericHeader{Key: name, Value: value}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only change in the function, we are not adding the otel header, so we can continue the trace.
My fault this time, working on a fix |
/retest |
Merged thanks. |
Otel needs a root spans, if that is not passed from external service, we should start one ourselves.
Also jobs shall retrieve these spans and not start new one.
For Jobs:
And for Kafka messages: