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

metrics-generator: use fanout storage to correctly update Prometheus remote write #2463

Merged
merged 3 commits into from
May 11, 2023

Conversation

kvrhdn
Copy link
Member

@kvrhdn kvrhdn commented May 11, 2023

What this PR does:

Fixes a bug in our usage of the Prometheus agent.DB and remote.Storage structures. When we append data onto agent.DB we also need to append it onto remote.Storage to ensure prometheus_remote_storage_highest_timestamp_in_seconds is updated (append implementation).

This timestamp is used to determine how many shards are needed, since this metric wasn't updated desired shards is always set to 1, even if remote write is running behind and needs to catch up.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

CHANGELOG.md Outdated
@@ -31,6 +31,7 @@ To make use of filtering, configure `autocomplete_filtering_enabled`.
* [ENHANCEMENT] Add support for IPv6 [#1555](https://github.com/grafana/tempo/pull/1555) (@zalegrala)
* [ENHANCEMENT] Add span filtering to spanmetrics processor [#2274](https://github.com/grafana/tempo/pull/2274) (@zalegrala)
* [BUGFIX] tempodb integer divide by zero error [#2167](https://github.com/grafana/tempo/issues/2167) (@kroksys)
* [BUGFIX] metrics-generator: use fanout storage to correctly update Prometheus remote write [#2463](https://github.com/grafana/tempo/issues/2463) (@kvrhdn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will an end-user understand this?
Suggest to focus more on the effect, which I think is Tempo will now use multiple connections to send samples as the rate gets higher.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point thanks! I tried to improve it somewhat

@09jvilla
Copy link
Contributor

@kvrhdn kvrhdn marked this pull request as ready for review May 11, 2023 18:25
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

lgtm

@kvrhdn kvrhdn merged commit ec3a8ad into grafana:main May 11, 2023
14 checks passed
@kvrhdn kvrhdn deleted the kvrhdn/metrics-generator-fanout branch May 11, 2023 19:24
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.

None yet

4 participants