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

[receiver/prometheus] feat: add Prometheus API server #33875

Closed
wants to merge 12 commits into from

Conversation

gracewehner
Copy link
Contributor

Description:

This is a new PR instead of this one: #32646 which has more relevant discussion.
This PR adds the Prometheus API server as part of the Prometheus receiver if enabled through the receiver config.
It supports calling the /config, /targets, /targets/metadata, /scrape_pools, and /metrics paths for more debugging information about the underlying Prometheus scrape manager and discovery manager.

Link to tracking Issue:
open-telemetry/wg-prometheus#63

Testing:
Added tests for the prometheus receiver configuration, for each path that the API supports, and for the server shutting down correctly. Also tested out with running the opentelemetry-collector agent with the prometheus receiver.

Documentation:
Added to the prometheus receiver README about how to configure the setting.

@gracewehner gracewehner requested a review from a team July 3, 2024 00:03
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Jul 3, 2024
@github-actions github-actions bot requested a review from Aneurysm9 July 3, 2024 00:03
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

Flags: make(map[string]string),
MaxConnections: maxConnections,
IsAgent: true,
Gatherer: prometheus.DefaultGatherer,
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tested this with multiple receivers? We've had collisions between metrics in different receivers in the past when using the global gatherer. You might need to use r.registerer, or make a gatherer specific to this receiver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dashpole I added a test for this with multiple receivers with it enabled. It uses the r.registerer below this but it looks like the DefaultGatherer doesn't have an equivalent function to just add the label like the registerer and is for having a whole different implementation. I made sure it works with both using prometheus.DefaultGatherer but different registerers

.chloggen/prometheusreceiverapiserver.yaml Outdated Show resolved Hide resolved
receiver/prometheusreceiver/README.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

Sorry for the slow review. Getting back from vacation

@dashpole
Copy link
Contributor

FYI @jesusvazquez

MaxConnections: maxConnections,
IsAgent: true,
Registerer: r.registerer,
Gatherer: prometheus.DefaultGatherer,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can change r.registerer to a *prometheus.Registry, which implements the Gatherer and Registerer interfaces, and then use it here. Maybe rename to r.registry as well

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 14, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/prometheus Prometheus receiver Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants