[api_v3][query] Change api_v3 http handler to use v2 query service #6459
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6459 +/- ##
==========================================
- Coverage 96.29% 96.27% -0.03%
==========================================
Files 371 370 -1
Lines 21173 21182 +9
==========================================
+ Hits 20389 20393 +4
- Misses 600 604 +4
- Partials 184 185 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cmd/query/app/apiv3/http_gateway.go
Outdated
| @@ -166,11 +160,16 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| request.RawTraces = rawTraces | |||
| } | |||
| trc, err := h.QueryService.GetTrace(r.Context(), request) | |||
| getTracesIter := h.QueryService.GetTraces(r.Context(), request) | |||
| trc, err := v1adapter.V1TracesFromSeq2(getTracesIter) | |||
There was a problem hiding this comment.
it doesn't make sense to convert to v1 model only to convert it back to OTLP in L113. The benefit of v2 storage here is that we can avoid any unnecessary transforms
There was a problem hiding this comment.
@yurishkuro yep - will fix. I was just trying to wire up the new query service first.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
cmd/query/app/apiv3/http_gateway.go
Outdated
| if h.tryHandleError(w, err, http.StatusInternalServerError) { | ||
| return | ||
| } | ||
| h.returnSpans(trc.Spans, w) | ||
| if len(trc) == 0 { | ||
| // TODO: should we return 404 if trace not found? |
There was a problem hiding this comment.
yes, I think this is the contract of v3 API (which is unfortunate since it doesn't match the Storage v2 API contract, but we'll keep it for backwards compatibility).
cmd/query/app/apiv3/http_gateway.go
Outdated
| @@ -166,11 +161,18 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| request.RawTraces = rawTraces | |||
| } | |||
| trc, err := h.QueryService.GetTrace(r.Context(), request) | |||
| getTracesIter := h.QueryService.GetTraces(r.Context(), request) | |||
| aggrTracesIter := jptrace.AggregateTraces(getTracesIter) | |||
There was a problem hiding this comment.
why do we need to aggregate here? If QS needs to apply adjustments it would already aggregate, otherwise it will return raw chunks, and this API can just pass them through to the caller, it's not require to recombine them.
cmd/query/app/apiv3/http_gateway.go
Outdated
| // TODO how do we distinguish internal error from bad parameters for FindTrace? | ||
| if h.tryHandleError(w, err, http.StatusInternalServerError) { | ||
| return | ||
| } | ||
| var spans []*model.Span | ||
| combinedTrace := ptrace.NewTraces() |
There was a problem hiding this comment.
this should have a serious TODO, since it's clearly the wrong thing todo now.
|
I added a sketch of the issue for chunked encoding #6467 |
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]>
Signed-off-by: Mahad Zaryab <[email protected]>
| @@ -8,16 +8,14 @@ | |||
| "scope": {}, | |||
| "spans": [ | |||
| { | |||
| "endTimeUnixNano": "11651379494838206464", | |||
There was a problem hiding this comment.
@yurishkuro do you know where this timestamp was coming from before?
mahadzaryab1
changed the title
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test