-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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] Add Target Info API #23244
Conversation
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
…tor-contrib into feat/promAPI
…tor-contrib into feat/promAPI
…tor-contrib into feat/promAPI
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
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.
We should not expose yet another HTTP endpoint/server per component, we should use https://github.com/open-telemetry/opentelemetry-collector/tree/main/extension/zpagesextension
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.
Per author comment, I have to block this since I want my comment to be considered, see #30135 (comment)
Thanks @Aneurysm9 for re-opening this! My suggestion would be to re-use the Prometheus API struct with agent mode set to true, so that we get the benefit of not needing to have as much duplicated code for the API internals from the Prometheus repo and this code drifting from the main Prometheus branch. I agree we don't want to add any additional API paths that don't apply to the Prometheus Receiver. With the API from the Prometheus repo, all paths with I tried this below as a rough POC and verified it works. This sets up the API in the same way as the Prometheus web package, which sets up the API and hosts it in addition to hosting the UI. We can do the same, but without adding in any of the UI-related code for serving the react app paths:
|
Hope we are reasonable and not need to block for a review to be considered
Hi @Aneurysm9 any update on this PR? I have confirmed it's possible to host the Prom UI separately on a different port through golang and re-route the API calls to the prom receiver's API port, so this PR will still work well with the out-of-the-box Prom react app |
I would also prefer not to copy as much of the prometheus codebase if we can avoid it. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Hi @Aneurysm9 friendly ping for this PR. I am happy to help with any changes needed for this PR to go in |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@gracewehner I think we're going to run into issues with Prometheus having duplicated some Collector code. I get the following error, even after removing any explicit reference to the Prometheus
I suspect we're caught in a loop where Prometheus duplicates code from the Collector because we import code from them which causes problems for them updating the code, which prevents us from further importing code that references that duplicated code, causing us to duplicate code from them. Since there's just a single module on the Prometheus side we don't have an option to |
Based on discussion at the WG last week I have created prometheus/prometheus#13932 to remove the conflicting feature gate registration from the copied translation packages in |
Thanks @Aneurysm9 for investigating, I was also seeing that issue. I had also been working on a full PR for the alternative API approach I had mentioned above and having it as an extension. I just made the PR here: #32646. We can discuss the approaches in the meeting tomorrow |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description: Adds ability to provide
confighttp.HTTPServerSettings
to theprometheus
receiver that will be used to expose a subset of the Prometheus API. At present this only includes the/targets
resource that will return information about active and discovered scrape targets, including debugging information typically not available without verbose debug logging.