[query] Use OTEL's helpers for http server #6121
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6121 +/- ##
==========================================
- Coverage 96.43% 96.42% -0.02%
==========================================
Files 353 353
Lines 20128 20141 +13
==========================================
+ Hits 19410 19420 +10
- Misses 532 534 +2
- Partials 186 187 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
what was the problem with http in the first diff? Did you find a workaround? |
mahadzaryab1
changed the title
cmd/query/app/apiv3/http_gateway.go
Outdated
| // traceMiddleware := otelhttp.NewHandler( | ||
| // otelhttp.WithRouteTag(route, handler), | ||
| // route, | ||
| // otelhttp.WithTracerProvider(h.Tracer)) |
There was a problem hiding this comment.
@yurishkuro I tried commenting out this middleware to see if the GRPC tests would pass but it looks like the grpc v2 tests are running into the same issue of getting stuck in a loop of WriteSpan. Any idea why that's happening and if there's a way around it?
There was a problem hiding this comment.
why are grpc tests affected by http changes? We already merged grpc PR on green CI.
There was a problem hiding this comment.
its not the grpc server tests - its the grpc storage tests (https://github.com/jaegertracing/jaeger/actions/runs/11526027865/job/32089447738?pr=6121)
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
force-pushed
the
http-otel
branch
from
630f0c9
to
e5fa520
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
changed the title
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
|
@yurishkuro looks like my latest commit performs a lot of the same changes as in #6128. How do you want to proceed? Should I revert my last commit? |
This reverts commit a1beee7. Signed-off-by: Mahad Zaryab <[email protected]>
## Which problem is this PR solving? - Prequel for #6121 ## Description of the changes - Consolidate business logic handlers into a single function `initRouter` - Move tenancy handling to the same function, remove it from API and APIv3 handlers ## How was this change tested? - `go test ./cmd/query/app/...` --------- Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
| route, | ||
| otelhttp.WithTracerProvider(aH.tracer)) | ||
| return router.HandleFunc(route, traceMiddleware.ServeHTTP) | ||
| handler = traceResponseHandler(handler) |
There was a problem hiding this comment.
TODO traceResponseHandler should be moved to initRouter, it can apply to the whole server, not just API handler
There was a problem hiding this comment.
@yurishkuro do we want to do that in this PR or a different one?
There was a problem hiding this comment.
probably another, already too many changes here to keep track of
cmd/query/app/server.go
Outdated
| server.TLSConfig = tlsCfg | ||
| } | ||
| return server, nil | ||
| } |
There was a problem hiding this comment.
misleading diff display by GH - this part above did not actually change
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
cmd/query/app/server_test.go
Outdated
| @@ -855,3 +859,104 @@ func TestServerHTTPTenancy(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestServerHTTP_TracesRequest(t *testing.T) { | |||
There was a problem hiding this comment.
This is a great test, but it highlights a problem. I recommend creating a separate PR with just this test, and it will fail on main, which means we are changing something about the behavior.
There was a problem hiding this comment.
@yurishkuro this is rebased now
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
cmd/query/app/server_test.go
Outdated
| httpEndpoint: ":8080", | ||
| grpcEndpoint: ":8080", | ||
| queryString: "/api/traces/123456aBC", | ||
| expectedTrace: "/api/traces/{traceID}", |
There was a problem hiding this comment.
why are we loosing this trace? That was my point of adding the test to main first - it should not change in this PR.
There was a problem hiding this comment.
@yurishkuro We're losing this trace at the moment because removed the call to otelhttp.NewHandler which was taking a tracer. If we don't remove that call then we run into the issue of double instrumentation that we were seeing earlier.
There was a problem hiding this comment.
so how do we explain that? The OTEL helper is supposed to add tracing middleware, why is it not working?
There was a problem hiding this comment.
@yurishkuro The OTEL helper does add tracing. However, when the ports are the same, we're not using the OTEL helper to initialize the server and are our falling back to our custom implementation. We were previously using otelhttp.NewHandler in http_handler.go and http_gateway.go which was previously rendering this trace. We removed it from there because then we were running into the case of double instrumentation when the ports were different and we were using the OTEL helpers.
There was a problem hiding this comment.
Thinking more about this - is there a reason why we can't just use OTEL's helper to create the server even if the ports are the same? We can still use a different flow when we start the listener.
There was a problem hiding this comment.
@yurishkuro I was actually wondering why we can't just use the server created by ToServer for the legacy flow as well?
There was a problem hiding this comment.
How would we co-locate http and grpc servers on the same port in that case? We could if the listener is still done separately by our code.
There was a problem hiding this comment.
@yurishkuro The listener is initialized separately and it already has a check for separate ports (see https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/server.go#L289). So can we just simplify the logic of creating the server to always create it using ToServer and then we can handle the legacy behaviour inside ToListener.
There was a problem hiding this comment.
give it a try. Or just add tracing handler to Legacy function and we can merge this PR, then try the listener separately.
There was a problem hiding this comment.
@yurishkuro I removed the bifurcation for the creation of the server and it looks to be working as expected which makes sense. We can simply rely on OTEL's helpers to create the server. We only need special handling if the ports are the same (and that handling exists already in initListener). I'll do the same for the GRPC server in a follow-up PR.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
This reverts commit c5f34d9.
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test