Skip to content

[Query-frontend] Fix metrics streaming#4624

Merged
joe-elliott merged 9 commits intografana:mainfrom
joe-elliott:fix-the-next-thing
Jan 29, 2025
Merged

[Query-frontend] Fix metrics streaming#4624
joe-elliott merged 9 commits intografana:mainfrom
joe-elliott:fix-the-next-thing

Conversation

@joe-elliott
Copy link
Copy Markdown
Collaborator

@joe-elliott joe-elliott commented Jan 27, 2025

What this PR does:
Metrics diff streaming relied on using the start/end timestamps of input series and labels to determine which output series were impacted. In hindsight is quite obvious that this will only work in the most trivial of examples. Consider a metrics quantile stream: The input series are all labeled like histograms, but the output series are labeled like quantiles.

The engine can technically calculate which output series are impacted by which input series, but this would be increasingly complicated as we added additional metrics function. It would also be quite brittle and require constant upkeep for every function. Instead we have settled on doing a simple diff in the frontend between the previous metrics and the current one.

Notes for the reviewer:

I am currently using prom label string equality to match previous and current series. I believe this works, but looking for confirmation from someone with more experience.

Checklist

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

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@adrapereira
Copy link
Copy Markdown

I am currently using prom label string equality to match previous and current series. I believe this works, but looking for confirmation from someone with more experience.

I've had issues with this in Grafana but was generating my own string based on the Labels map, so had to sort it before generating the string. As long as the PromLabels values are generated consistently across diffs this should work 👍 .

Comment thread modules/frontend/combiner/metrics_query_range.go Outdated
Copy link
Copy Markdown
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

Looking good to me. Good approach for diffing the sorted slices. Using prom labels is ok, internally it is the key in the SeriesSet and used for other combine/compare operations like here. If there is an issue with it, would rather have the consistent usage that can all be fixed together than try something different here.

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott merged commit 8e74079 into grafana:main Jan 29, 2025
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.

3 participants