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] Fix handling of empties and nils + other fixes #3440

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Feb 28, 2024

What this PR does:
Basically this overhauls the handling of timeseries labels for metrics queries. There are several fixes in here, but it also lines up behavior with: grafana/grafana#83619

  • Empty strings were previously dropped but now they are preserved. For example: {rootName=""} | rate() by (rootName). This would look like a completely empty labelset in the response and cause issues downstream. Now it's preserved as the single label rootName with an empty string value.
  • When all labels are nil, we were backfilling a single label with the text "" to make Grafana happy like. This was actually premature specialization for the prometheus-compatible API, and now that we have the new dedicated Tempo-UI we can fix this. Now nils are dropped entirely and handled in the frontend. They will become {} in the Tempo UI and the query name in the Prometheus UI (a fallback method). Note: the EmitDefaults:true is part of this so that Tempo always outputs an empty array in the json (and not nil).
  • Fix sorting. Previously we only sorted during the conversion to Prometheus format. Also fix sorting to handle non-strings and multiple label values correctly.
  • Fix default step calculation and error handling.

Which issue(s) this PR fixes:
Fixes #3429

Checklist

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

@mdisibio
Copy link
Contributor Author

Requires: grafana/grafana#83619

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM

@mdisibio mdisibio merged commit 781bcaf into grafana:main Feb 29, 2024
14 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.

[TraceQL Metrics] Unexpected error when rating by rootName/rootServiceName
2 participants