Skip to content

fix: fix MetricsAggregator check for metric sameness #11508

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

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Jan 9, 2024

Fixes #11451

A refactor of the Agent API passes metrics as protobufs, which include pointers to label name/value pairs. The aggregator tested for sameness by doing a shallow compare of label values, which for different stats reports would compare unequal because the pointers would be different.

This fix does a deep compare.

While testing I also noted that we neglect to compare template names. This is unlikely to have caused any issue in practice, since the combination of username/workspace is unique, but in the context of comparing metric labels we should do the comparison.

If a user creates a workspace, deletes it, then recreates from a different template, we could in principle have reported incorrect stats for the old template.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spikecurtis and the rest of your teammates on Graphite Graphite

@spikecurtis spikecurtis requested review from Emyrk and mtojek January 9, 2024 09:24
@spikecurtis spikecurtis force-pushed the spike/11451-metrics-aggregator-is branch from c136bbe to 1734244 Compare January 9, 2024 09:30
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Left a few nit-picks

@spikecurtis spikecurtis force-pushed the spike/11451-metrics-aggregator-is branch from 1734244 to 3259e39 Compare January 9, 2024 11:10
@spikecurtis spikecurtis merged commit fdd60d3 into main Jan 9, 2024
@spikecurtis spikecurtis deleted the spike/11451-metrics-aggregator-is branch January 9, 2024 11:21
Copy link
Contributor Author

Merge activity

@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2024
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Good find

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus metrics endpoint returns 500 instead of metrics
3 participants