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

feat: Allow partial traces to be returned #3941

Merged
merged 24 commits into from
Aug 26, 2024

Conversation

javiermolinar
Copy link
Contributor

@javiermolinar javiermolinar commented Aug 6, 2024

What this PR does:
Allow return partial traces for traces API V2

This is a prequel to a more intelligent span-dropping algorithm. For version 1 of Traces API, an error is returned when the trace size exceeds the global max bytes limit. With this PR, V2 will return a partial trace, once the limit has been reached no more spans will be added.

The change has been propagated from the front-end way down the read path.

How the response looks like:

{
  "trace": {
    "resourceSpans": [
    ]
  },
  "traceType": "PARTIAL",
  "message": "Trace exceeds maximun size of 10 bytes, a partial trace is returned"
}

Checklist

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

Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Debating on if it we should continue the FindTraceByIDV2 path all the way to the Finder interface and fork a new Combiner, instead of extending v1 with the bool. I think as we add smarter logic to the combiner, we will ultimately want that. But this looks great for now and going ahead and approving.

Left one nit, non-blocking, but something to consider.

pkg/model/trace/combine.go Outdated Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

a good start, but it needs a way to signal to the caller that a partial trace is returned. this is going to be tough b/c of how many places we truncate the trace based on size.

perhaps we need a sentinel error that signals the trace is truncated but otherwise valid? i'm open to other solutions.

@javiermolinar
Copy link
Contributor Author

a good start, but it needs a way to signal to the caller that a partial trace is returned. this is going to be tough b/c of how many places we truncate the trace based on size.

perhaps we need a sentinel error that signals the trace is truncated but otherwise valid? i'm open to other solutions.

Do you have an example where this could be problematic? My rationale was that instead of returning an error, stop concating more traces. For the client, we will add a message in the TraceByIDResponse

@joe-elliott
Copy link
Member

My rationale was that instead of returning an error, stop concating more traces. For the client, we will add a message in the TraceByIDResponse

This is what I mean. I was thinking about an error as a way to internally signal that the trace had been partially dropped, but returning a 200 to the client with a message.

For instance how do we bubble up that a trace was truncated here:

https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet4/block_findtracebyid.go#L266-L271

Its possible the right choice is to reduce where we are checking the max trace size?

And then how are communicating from the querier -> query-frontend that a trace is partial?

pkg/tempopb/tempo.proto Outdated Show resolved Hide resolved
pkg/model/v1/segment_decoder.go Outdated Show resolved Hide resolved
modules/querier/querier.go Outdated Show resolved Hide resolved
modules/querier/querier.go Outdated Show resolved Hide resolved
modules/querier/querier.go Outdated Show resolved Hide resolved
pkg/model/trace/combine.go Outdated Show resolved Hide resolved
pkg/model/trace/combine.go Outdated Show resolved Hide resolved
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.

3 participants