-
Notifications
You must be signed in to change notification settings - Fork 203
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
Upgrade prometheus #794
Upgrade prometheus #794
Conversation
b8609c0
to
792ef4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I added a few minor comments. As long as Matt's comments are also addressed, I'd be happy to approve it.
@@ -222,6 +223,23 @@ func (a *appender) AppendHistogram(ref storage.SeriesRef, l labels.Labels, t int | |||
return ref, multiErr | |||
} | |||
|
|||
func (a *appender) AppendCTZeroSample(ref storage.SeriesRef, l labels.Labels, t, ct int64) (storage.SeriesRef, error) { | |||
if a.start.IsZero() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this from some existing code? Looking at the prometheus code I don't see similar logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a new appender method AppendCTZeroSample
and I'm implementing it here in fanout to match the way other appender methods are implemented. This is not copied from anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, maybe a question on how ctzappender is used but I am not up to date on what it really does.
a875a23
to
25d17b7
Compare
I believe you cannot even use it in Alloy right now, you'd need to enabled it via https://github.com/prometheus/prometheus/blob/07355c9199e0913a76e1a992816e63b0683d16e7/cmd/prometheus/main.go#L224 - which we are not yet exposing. It's for the timestamp of the creation of the series, which may or may not become a popular feature: https://github.com/prometheus/proposals/blob/main/proposals/2023-06-13_created-timestamp.md |
PR Description
This upgrades the Prometheus dependency. Some required, notable changes:
scrape_protocols
was added toprometheus.scrape
enable_protobuf_negotiation
will remain working the same as previous, but it is now deprecated andscrape_protocols
should be used instead.github.com/open-telemetry/opentelemetry-collector-contrib/receiver/prometheusreceiver
as it depended on an incompatible Prometheus version.go.opentelemetry.io/collector/featuregate
as we had to work around the problem where Prometheus and OTEL try to register duplicate OTEL feature flags and result in panics."github.com/prometheus/prometheus/model/textparse"
->"github.com/prometheus/common/model"
AppendCTZeroSample
method in appenders where needed. It is not exposed in any config right now to keep this PR smaller in scope.Which issue(s) this PR fixes
Fixes #227
Notes to reviewer
Tested this with adding / removing discoverers at runtime and checking that their metrics are unregistered and no panics happen. For example one test scenario:
We can see these metrics:
After renaming the file discovery component and reloading the config we can see:
PR Checklist