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

[Traceql Metrics] PR 2 - API #3252

Merged
merged 28 commits into from
Jan 12, 2024
Merged

[Traceql Metrics] PR 2 - API #3252

merged 28 commits into from
Jan 12, 2024

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Dec 19, 2023

What this PR does:

This is the result of a collaboration effort and requires #3227 for the underlying work of providing the data. Here we build out the API for metrics in Tempo.

  • Update API and proto definitions
  • Build out the generator API
  • Wire up the generator API to queriers
  • Handle the query and backend sharding on the frontend

Notes
This is one entry in a set of chained PRs. The full body of work has been split into separate buckets to make reviews and updates more manageable.

  1. [Traceql Metrics] PR 1 - Engine #3251
  2. [Traceql Metrics] PR 2 - API #3252
  3. [Traceql Metrics] PR 3 - Trace ID sharding #3258

Checklist

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

@mdisibio mdisibio mentioned this pull request Dec 19, 2023
3 tasks
@mdisibio mdisibio mentioned this pull request Jan 9, 2024
3 tasks
Base automatically changed from traceql-metrics-1-engine to main January 11, 2024 15:20
@@ -305,7 +304,7 @@ func newSpansetFilter(e FieldExpression) *SpansetFilter {
func (*SpansetFilter) __spansetExpression() {}

func (f *SpansetFilter) evaluate(input []*Spanset) ([]*Spanset, error) {
f.outputBuffer = f.outputBuffer[:0]
var outputBuffer []*Spanset
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is required to make the traceql eval callback safe for concurrent use (so we can execute the query against multiple blocks in parallel). This was ok previously because: regular searches were single threaded or the query was compiled for each block and they got their own AST (and SpansetFilters). But now for metrics the AST keeps more state and it should be more efficient to reuse the same AST across all blocks. Otherwise it would have to create and squash repeatedly the same timeseries from each block to get the final results.

Copy link
Contributor

@zalegrala zalegrala left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@zalegrala zalegrala merged commit f4b2a4b into main Jan 12, 2024
16 checks passed
@zalegrala zalegrala deleted the traceql-metrics-2-api branch January 12, 2024 13:48
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.

2 participants