feature: [trace_id idx] add trace_id index to accelerate query execution #22
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a trace_id index stream to accelerate query execution for trace lookups. The feature introduces an index stream that stores trace IDs with their start timestamps to enable faster trace retrieval by avoiding full time range scans.
- Adds trace_id index constants and partitioning configuration
- Implements fast path trace lookup using the index stream with fallback to step-based search
- Refactors field name generation for events and links to append index suffixes correctly
Reviewed Changes
Copilot reviewed 6 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/protoparser/opentelemetry/pb/trace_fields.go | Defines constants for trace_id index stream name, field name, and partition count |
| go.mod | Adds fastcache dependency for trace ID deduplication |
| app/vtselect/traces/query/query.go | Implements fast path trace lookup using index stream and refactors existing search logic |
| app/vtselect/traces/jaeger/model_test.go | Adds test for removeArrayIndex function |
| app/vtselect/traces/jaeger/model.go | Refactors field parsing to extract attribute names and indices correctly |
| app/vtinsert/opentelemetry/opentelemetry.go | Creates trace_id index entries and refactors field generation with suffix support |
Comments suppressed due to low confidence (1)
app/vtselect/traces/jaeger/model.go:275
- The function name 'extraAttributeNameAndIndex' is unclear. It should be named more descriptively, such as 'extractAttributeNameAndIndex' or 'parseAttributeNameAndIndex'.
func extraAttributeNameAndIndex(input string) (string, string) {
| qStr := fmt.Sprintf(otelpb.TraceIDField+": %q", traceID) | ||
| // possible partition | ||
| // query: {trace_id_idx="xx"} AND trace_id:traceID | ||
| qStr := fmt.Sprintf(`{%s="%d"} AND %s:=%q | fields _time`, otelpb.TraceIDIndexStreamName, xxhash.Sum64String(traceID)%otelpb.TraceIDIndexPartitionCount, otelpb.TraceIDIndexFieldName, traceID) |
There was a problem hiding this comment.
The query uses ':=' operator but the constant name suggests it should be ':'. The operator ':=' is typically for assignment, not field matching. This should likely be ':' for field matching.
| qStr := fmt.Sprintf(`{%s="%d"} AND %s:=%q | fields _time`, otelpb.TraceIDIndexStreamName, xxhash.Sum64String(traceID)%otelpb.TraceIDIndexPartitionCount, otelpb.TraceIDIndexFieldName, traceID) | |
| qStr := fmt.Sprintf(`{%s="%d"} AND %s:%q | fields _time`, otelpb.TraceIDIndexStreamName, xxhash.Sum64String(traceID)%otelpb.TraceIDIndexPartitionCount, otelpb.TraceIDIndexFieldName, traceID) |
app/vtselect/traces/query/query.go
Outdated
| return time.Time{}, nil | ||
| } | ||
|
|
||
| // findSpanByTraceID searches for spans from now to 0 time with steps. |
There was a problem hiding this comment.
The comment says 'findSpanByTraceID' but the actual function name is 'findSpansByTraceID' (plural). The comment should match the function name.
| // findSpanByTraceID searches for spans from now to 0 time with steps. | |
| // findSpansByTraceID searches for spans from now to 0 time with steps. |
| } | ||
|
|
||
| // found result, perform extra search for traceMaxDurationWindow and then break. | ||
| return time.Unix(traceIDStartTimeInt/1e9, traceIDStartTimeInt%1e9), nil |
There was a problem hiding this comment.
The magic numbers '1e9' should be replaced with a named constant like 'time.Second.Nanoseconds()' or '1000000000' with a comment explaining the nanosecond conversion.
| return time.Unix(traceIDStartTimeInt/1e9, traceIDStartTimeInt%1e9), nil | |
| return time.Unix(traceIDStartTimeInt/time.Second.Nanoseconds(), traceIDStartTimeInt%time.Second.Nanoseconds()), nil |
1 participant
Describe Your Changes
Fix #21
Checklist
The following checks are mandatory: