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

Fix issue where applying httpgrpc tracing middleware results in all HTTP requests having duplicate trace spans #451

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 13, 2023

What this PR does:

This PR fixes the issue where applications that use the httpgrpc tracing middleware (through middleware.InitHTTPGRPCMiddleware) would have traces for all incoming HTTP calls duplicated.

For example, note the duplicate HTTP POST - api_v1_push spans in this trace from a Mimir distributor:

Screenshot 2023-12-13 at 12 50 20 pm

The fix changes the HTTP middleware to a gRPC interceptor that is then able to accurately filter incoming requests and apply the additional information only to httpgrpc requests.

One downside of this is that we'll convert the request to a http.Request twice now: once in the interceptor, and again in the httpgrpc handler, but this seems like the neatest solution.

Related: #353

Which issue(s) this PR fixes:

(none)

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Member

@francoposa francoposa left a comment

Choose a reason for hiding this comment

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

LGTM!
If you were feeling particularly enterprising, you could figure out how to use the an OTEL tracer in the tests instead of this jaeger one.
The jaeger tracer is the one dskit used to use for opentracing before it was converted to OTEL. It stuck around in this test and maybe others but isn't actually used in the application anymore

@charleskorn
Copy link
Contributor Author

If you were feeling particularly enterprising, you could figure out how to use the an OTEL tracer in the tests instead of this jaeger one.
The jaeger tracer is the one dskit used to use for opentracing before it was converted to OTEL. It stuck around in this test and maybe others but isn't actually used in the application anymore

Sounds good, I'll take a look at this in a separate PR.

@charleskorn charleskorn merged commit 84f5540 into main Dec 13, 2023
3 checks passed
@charleskorn charleskorn deleted the charleskorn/httpgrpc-tracing branch December 13, 2023 22:30
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

2 participants