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

Dynamic metrics group by multiple attributes #2582

Merged
merged 5 commits into from
Jul 6, 2023

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Jun 22, 2023

What this PR does:
Updated /api/metrics/summary to support grouping by mulitple attributes. The groupBy parameter can be a comma delimited list of attributes. Response was changed to return a slice of key/value pairs, in the same order that they are specified in groupBy. A nil is returned for any missing value. Originally it was just the values, but I thought it was better to include the key instead of putting the onus on the caller to know what to expect.

This is a draft because it should go after #2579 which also has changes in this area.

Which issue(s) this PR fixes:
Fixes n.a

Checklist

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

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.

This looks like a good start to me. Do you want to plumb through the frontend, or leave that for another PR?

modules/generator/processor/localblocks/processor.go Outdated Show resolved Hide resolved
@mdisibio mdisibio marked this pull request as ready for review June 26, 2023 20:21
@mdisibio
Copy link
Contributor Author

mdisibio commented Jun 27, 2023

This looks like a good start to me. Do you want to plumb through the frontend, or leave that for another PR?

Can you clarify what you mean? It already works end-to-end by reusing the groupBy parameter to be a comma-delimited list. The output proto/json is also updated to return the slice of group values.

Note I chose comma because it's a common delimiter, and it's already excluded from being part of an attribute name in regular queries by the code here: https://github.com/grafana/tempo/blob/main/pkg/traceql/lexer.go#L253

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.

Sorry, I meant to approve yesterday. This looks good to me with one comment about whitespace on the query.

pkg/traceqlmetrics/metrics.go Show resolved Hide resolved
return results, nil
}

func lookup(needles []traceql.Attribute, haystack map[traceql.Attribute]traceql.Static) traceql.Static {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, I love this signature.

Copy link
Contributor Author

@mdisibio mdisibio Jun 28, 2023

Choose a reason for hiding this comment

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

Yeah it was feeling clunky but I think those parameter names saved it. :)

message SpanMetrics {
repeated RawHistogram latency_histogram = 1;
TraceQLStatic static = 2;
repeated KeyValue series = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we also include a status code to indicate control messages like not-found?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or possibly on the KeyValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When an attribute is not found it will return a traceql static with Nil type (empty value). This works because real nils are not possible (against spec but also not persisted).

@mdisibio mdisibio merged commit b42dc9b into grafana:main Jul 6, 2023
13 checks passed
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