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 in the "scrape" package can now be registered with a non-default registry #12958

Merged

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Oct 9, 2023

A Registerer is passed to the scrape Manager, and all scrape metrics register with it. For now the registry which we pass to the scrape Manager is still the global one.

I tested this locally by running a Prometheus which scrapes itself. I checked the https://localhost:9090/metrics metrics address for a local Prometheus and I see all scrape metrics which I also see using a binary from the main branch.

* A registerer is passed to the scrape Manager,
and all scrape metrics register with it.
* For now the registry which we pass to the scrape
Manager is still the global one.

Signed-off-by: Paulin Todev <[email protected]>
@ptodev ptodev force-pushed the prefer-to-not-register-metrics-globally-scrape branch from c1aceae to 5752050 Compare October 11, 2023 15:19
@beorn7
Copy link
Member

beorn7 commented Oct 14, 2023

Will have a look ASAP. (Still on my way back from SREcon. @ptodev it was nice meeting you on my way back in London. ;) )

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

OK, I guess nothing surprising in this change. I give it a few days to see if @roidelapluie and @LeviHarrison as the repo maintainers have any concerns.

@ptodev
Copy link
Contributor Author

ptodev commented Oct 23, 2023

Will have a look ASAP. (Still on my way back from SREcon. @ptodev it was nice meeting you on my way back in London. ;) )

@beorn7 Thank you so much for the advice, for the review, and for coming over to London! It was great to meet up! 😊

I also raised #13023 to make a similar change to the SD packages. It is not urgent, but I would appreciate if you could take a look sometime please.

@beorn7 beorn7 merged commit f2e02c5 into prometheus:main Oct 24, 2023
24 checks passed
Aneurysm9 added a commit to Aneurysm9/prometheus that referenced this pull request Feb 19, 2024
PR prometheus#12958 introduced the ability to provide a registerer when creating a
scrape manager, but this did not account for the possibility of using
multiple scrape managers with the same registerer.  This change simply
ignores the `prometheus.AlreadyRegisteredError` when attempting to
register scraper metric collectors.

Fixes prometheus#13507

Signed-off-by: Anthony J Mirabella <[email protected]>
dmitryax pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Mar 25, 2024
…30934)

Fixes
#30883

Prometheus PR we need to adapt to:
prometheus/prometheus#12738
The configuration option we were using to enable protobuf has been
removed, so this PR removes the `enable_protobuf_negotiation`
configuration on the prometheus receiver. In its place, the prometheus
server now has a config option, `scrape_protocols`, which specifies
which protocols it uses, including protobuf. Use
`config.global.scrape_protocols = [ PrometheusProto,
OpenMetricsText1.0.0, OpenMetricsText0.0.1, PrometheusText0.0.4 ]` to
enable protobuf scraping instead of using our option.

Prometheus PR we need to adapt to:
prometheus/prometheus#12958
We now need to pass a prometheus.Registerer to the scrape manager to
collect metrics about scrapes. This PR disables the collection of these
scrape metrics from the prometheus server. We did not use the default
registry, so we were previously collecting these metrics and never
exposing them. This PR passes a no-op registerer to prevent using memory
to store those metrics. In the future, we can consider using the
prometheus -> OTel bridge to export these with the self-observability
pipeline.
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