Skip to content
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

Otel tracing MVP #4188

Merged
merged 3 commits into from Dec 11, 2023
Merged

Otel tracing MVP #4188

merged 3 commits into from Dec 11, 2023

Conversation

gotgelf
Copy link
Contributor

@gotgelf gotgelf commented Dec 11, 2023

This PR represents the first step in the incremental integration of OpenTelemetry into our application. Following the recommendations from the review of PR #3701, this PR focuses on a minimal viable product for OpenTelemetry instrumentation.

Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on!

Are you comfortable with rewriting Git history? I would really appreciate if you could move the vendoring changes into their own separate commit so I can skip over it when reviewing the changes. GitHub's web interface lags horribly and eats nearly a gig of RAM just for this one browser tab when I try to review this PR.

semconv.ServiceVersionKey.String(version.Version),
)

exp, err := autoexport.NewSpanExporter(context.Background())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plumb your contexts through!

Suggested change
exp, err := autoexport.NewSpanExporter(context.Background())
exp, err := autoexport.NewSpanExporter(ctx)

Comment on lines 177 to 183
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Create a custom operation name
operationName := r.Method + " " + r.URL.Path

otelHandler := otelhttp.NewHandler(next, operationName)
otelHandler.ServeHTTP(w, r)
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing a new handler on each request is more expensive than necessary. Given that this is a code path which is executed for every incoming HTTP request, I feel a little optimization is not premature.

Peeking under the hood of otelhttp, the operation parameter is only used as an input to the span name formatter function, along with the request being handled. The default formatter just passes through the operation name, which means that this otelHandler is just formatting the span name from the request in a very roundabout way.

Suggested change
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Create a custom operation name
operationName := r.Method + " " + r.URL.Path
otelHandler := otelhttp.NewHandler(next, operationName)
otelHandler.ServeHTTP(w, r)
})
return otelhttp.NewHandler(next, "",
otelhttp.WithSpanNameFormatter(func(_ string, r *http.Request) string { return r.Method + " " + r.URL.Path }))

Signed-off-by: gotgelf <gotgelf@gmail.com>
Signed-off-by: gotgelf <gotgelf@gmail.com>
@gotgelf
Copy link
Contributor Author

gotgelf commented Dec 11, 2023

Are you comfortable with rewriting Git history?

Yes, done.

@gotgelf gotgelf requested a review from corhere December 11, 2023 20:21
Signed-off-by: gotgelf <gotgelf@gmail.com>
Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants