Parquet gateway tracing configuration update and trace span enhancements#84
Open
ringerc wants to merge 12 commits intothanos-io:mainfrom
Open
Parquet gateway tracing configuration update and trace span enhancements#84ringerc wants to merge 12 commits intothanos-io:mainfrom
ringerc wants to merge 12 commits intothanos-io:mainfrom
Conversation
ringerc
force-pushed
the
tracing-part-1
branch
3 times, most recently
from
370b5eb to
c406aa4
Compare
ringerc
force-pushed
the
tracing-part-1
branch
2 times, most recently
from
0878fda to
c6a5408
Compare
Author
|
Triggered a LLM review in a copy of this PR branch since limitations of the platform prevent one from being done in this branch. Looks like some useful commentary, checking....
I'll check those and rebase them in. |
Author
|
Fixes pushed:
Also ran |
GiedriusS
reviewed
Mar 23, 2026
| } | ||
| } | ||
| var seriesCount, sampleCount int64 | ||
| switch results := res.Value.(type) { |
Member
There was a problem hiding this comment.
Why can't we use computeResultMetrics here?
Author
There was a problem hiding this comment.
It'd add a second pass over the data, instead of doing the counting as data is processed.
If that's not considered a concern I can probably update this to extract computeResultMetrics to a shared package and use it in the gRPC handler too.
Author
There was a problem hiding this comment.
I'll push a commit to merge these.
| return newLazySeriesSet(ctx, q.selectFn, sorted, hints, matchers...) | ||
| } | ||
|
|
||
| func matchersToStringSlice(matchers []*labels.Matcher) []string { |
Member
There was a problem hiding this comment.
storepb.PromMatchersToString() does the same
Author
There was a problem hiding this comment.
storepb.PromMatchersToString returns a single string. This wants a []string for exposing as an otel array-type result.
Add Thanos-compatible --tracing.config and --tracing.config-file support for configuring tracing. Deprecate the existing Jaeger config flags. As Thanos lacks support for the STDOUT trace destination, support for it is omitted from the file-based trace configuration. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Memoizing tracer creation with sync.OnceValue could race with tracer configuration loading and initialization, unintentionally caching the no-op tracer. There's little to no benefit in caching the tracer anyway. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
This drops support for the STDOUT tracer as the Thanos file loader doesn't support it. If anyone cares, we could add a stdout provider to the Thanos file based tracer loader. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Add new "query.expr" trace span attribute to thanos.Query/QueryRange gRPC trace event, to record the query-text in the incoming query. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Add result.series and result.samples counts to the trace events for Query gRPC responses. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Add result.series and result.samples span attributes to Series gRPC response trace events. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Add a series.matchers attribute to trace spans for Series gRPC requests. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Use the same result metrics calculations for both gRPC and HTTP API request entrypoints. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Wrap result metrics injection into spans up into a single InjectResultMetrics call that takes the span and result object, simplifying caller paths. Signed-off-by: Craig Ringer <craig.ringer@enterprisedb.com>
Author
|
@GiedriusS I've updated the PR to address one of your comments (the other I don't think needs a change), simplify the code, and add histogram tracking. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes a number of tracing-related changes.
I used a LLM tool to assist with the development and testing of this change series, but only step by step and with plenty of manual review and editing. I haven't scrutinised the test cover as closely as the rest.
Switch to config-file based trace configuration
Fix incorrect tracer memoization
The tracer was being memoized when initialised, but this could lead to a race with trace initialisation that caused the noop tracer to be cached. Remove the unnecessary memoization.
Additional trace span attributes
query.exprtrace span attribute to thanos.Query/QueryRange gRPC trace event, to record the query-text in the incoming query.result.seriesandresult.samplescounts to the trace events for Query gRPC responses.result.seriesandresult.samplesspan attributes to Series gRPC response trace events.series.matchersattribute to trace spans for Series gRPC requests.