feat: add better instrumentation to querier methods using new metrics stats#7571
feat: add better instrumentation to querier methods using new metrics stats#7571javiermolinar wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR adds shared OpenTelemetry span helpers for the querier and wires them into several querier request paths to attach request parameters and the new “stats metrics” fields onto spans.
Changes:
- Introduces
start*/finish*helpers to consistently set querier span attributes and attachSearchMetrics/MetadataMetrics/TraceByIDMetricsto spans. - Updates multiple querier methods (trace-by-id, search/tags/tag-values, query-range) to use the new span helpers and record errors via a shared
finishQuerierSpan. - Adds focused unit tests validating that span attributes and
AdditionalMetricsare exported as span attributes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/querier/span_attrs.go | New helper functions to start/end spans and attach request attributes + metrics. |
| modules/querier/span_attrs_test.go | Tests that span attributes + metrics are correctly emitted (including AdditionalMetrics). |
| modules/querier/querier.go | Replaces ad-hoc span code with shared helpers across several querier RPC methods. |
| modules/querier/querier_query_range.go | Adds spans around QueryRange execution paths using the shared helpers. |
| .chloggen/instrument-querier-methods.yaml | Adds a changelog entry for the querier instrumentation change. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Expand for findings |
| ctx, span, tenantID, err := startTagsBlockSpan(ctx, "Querier.internalTagsSearchBlockV2", req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error extracting org id in Querier.BackendSearch: %w", err) | ||
| } | ||
| defer func() { finishQuerierSpan(span, err, resp.GetMetrics()) }() |
stoewer
left a comment
There was a problem hiding this comment.
I left a few minor comments. Otherwise this looks good 👍
|
|
||
| func (q *Querier) SearchTagValuesBlocks(ctx context.Context, req *tempopb.SearchTagValuesBlockRequest) (*tempopb.SearchTagValuesResponse, error) { | ||
| func (q *Querier) SearchTagValuesBlocks(ctx context.Context, req *tempopb.SearchTagValuesBlockRequest) (resp *tempopb.SearchTagValuesResponse, err error) { | ||
| ctx, span, _, err := startTagValuesBlockSpan(ctx, "Querier.SearchTagValuesBlocks", req) |
There was a problem hiding this comment.
The call to q.internalTagValuesSearchBlock(ctx, req) later in this function creates a span with the name Querier.internalTagsSearchBlockV2 that carries similar (identical?) information. This is a bit redundant. Should we remove the internal one?
The same pattern with similarly duplicated spans occurs in SearchTagsBlocks, SearchTagsBlocksV2, and SearchTagsBlocksV2 as well
| resp, err := q.store.SearchTagValues(ctx, meta, req, opts) | ||
| resp, err = q.store.SearchTagValues(ctx, meta, req, opts) | ||
| if err != nil { | ||
| return &tempopb.SearchTagValuesResponse{}, err |
There was a problem hiding this comment.
I think this overwrites the named return value resp that is used in the deferred call to finishQuerierSpan() above. But readerWriter.SearchTagValues() populates metrics also when the function returns an error. Due to this overwrite, the metrics are not captured by the deferred call to finishQuerierSpan()
I think the main culprit here are the named return values (resp *tempopb.SearchTagValuesResponse, err error) as those tend to cause maintenance hazards in longer functions. Not a must, but finding an alternative pattern to capture the error and stats in the finish call would be nice (but not worth it, if it makes the function complex in other ways)
| ) | ||
| } | ||
|
|
||
| func setQuerierSpanMetrics(span oteltrace.Span, metrics any) { |
There was a problem hiding this comment.
The type switch without default case can silently do nothing when additional metrics structs are added in the future. Instead of one function with a type switch, this function can be refactored into 3 small separate function for each type
What this PR does:
Add span traces for all querier methods with standardized attributes and the new stats implemented in #7504
Which issue(s) this PR fixes:
Fixes #
Checklist
.chloggen/(runmake chlog-new, ormake chlog-new FILENAME=<name>to override the default branch-name file; see.chloggen/README.md)