feat: expose a Traces api V2 #3912
Conversation
javiermolinar
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
There was a problem hiding this comment.
one Q about forking vs code reuse. to be clear i'm on the fence myself. i will trust whatever direction you want to take this.
should we go ahead and add a "message" field to the tracebyidresponse? since we know we want it?
The V2 always returns a proto message. This is pretty opinionated
is this true? it looks like you added code to the common combiner to respond to marshal as either proto or json.
The new NewTraceByIDV2 combiner now benefits from the common combiner
nice, i've been wanting to do this, but haven't had a chance.
I rather close this one and add it on a different PR
Yeah, this was true at some point in time, I guess I changed my mind 🫤 |
modules/frontend/combiner/common.go
Outdated
| } | ||
| bodyString = string(buff) | ||
| } else { | ||
| c.marshallingFormat = api.HeaderAcceptJSON |
There was a problem hiding this comment.
i find it a little strange to set this here. this feels like something that should be set on creation of the struct to control the response behavior and never changed.
i don't think it currently is causing a bug, but since its unexpected i could see how it would trip someone up.
There was a problem hiding this comment.
Yes, I know what it looks like. The reason for that is to avoid a scenario where the implementation of the combiner function does not set it and therefore we will return an empty header. Another reason is how difficult is to know what kind of combiner are we returning since we always use the Combiner interface to later on do a type assertion:
NewSearchTagValues(limitBytes).(GRPCCombiner[*tempopb.SearchTagValuesResponse])
I wonder if we could return directly
func NewSearchTags(limitBytes int) GRPCCombiner[*tempopb.SearchTagsResponse]
Nevertheless I have renamed it to httpMarshalingFormat to be more explicit
modules/frontend/combiner/common.go
Outdated
| bodyString, err = new(jsonpb.Marshaler).MarshalToString(final) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error marshalling response body: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return &http.Response{ | ||
| StatusCode: c.httpStatusCode, |
There was a problem hiding this comment.
it's silly we set this to 200 in all the combiners and then return it here. we should hardcode this to return 200 and only set c.httpStatusCode if there's an error
the combiner was brought together from ~3 different sources when i refactored the frontend. it created some warts like this b/c i wanted to change as little code as possible
this (and the above comment) suggest we really need a newCombiner(foo &genericCombiner) method that properly initializes these fields
wdyt?
There was a problem hiding this comment.
I'm really on the fence on this. Especially because you don't need to initiate the status code to make it work. In the first iteration, it won't stop and it will be replaced by the response's one. But it's "magic". I've added the method to make it more explicit
|
Should we add docs for this? |
Great call out. Yes! Javi, api docs are here: https://github.com/grafana/tempo/blob/main/docs/sources/tempo/api_docs/_index.md |
Hi @knylander-grafana. I've added the documentation. Some questions:
|
What this PR does:
This PR adds a new API endpoint for retrieving traces based on the trace id.
It includes the following changes:
- No longer returns a 404 when the trace is not found. Now it returns a 200 OK and an empty trace response
- The V2 always returns a proto message. This is pretty opinionated
- The
TraceByIDResponsenow wraps the trace. This leaves room for adding extra metadata in the response.- The response is now OTEL compatible
Implementation details
NewTraceByIDV2combiner now benefits from the common combinerPost PR
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]