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

Dependency: Update to Prometheus 0.300.* #36873

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ArthurSens
Copy link
Member

@ArthurSens ArthurSens commented Dec 17, 2024

Description

Supersedes #36642

This PR updates the prometheus/prometheus library in our go.mods to 0.300.* (which represents Prometheus 3.0).

It touches many go.mod files, but the Prometheus Receiver is the only component heavily affected by breaking changes.

@ArthurSens
Copy link
Member Author

The changes made to the scrape manager are a bit more involved than I expected. We'll need a new zap->slog adapter to replace the existing zap->go-kit adapter.

Help appreciated because the solution was not immediately obvious to me 😬

Copy link
Contributor

github-actions bot commented Jan 1, 2025

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 Jan 1, 2025
@npordash
Copy link

npordash commented Jan 9, 2025

The changes made to the scrape manager are a bit more involved than I expected. We'll need a new zap->slog adapter to replace the existing zap->go-kit adapter.

Help appreciated because the solution was not immediately obvious to me 😬

@ArthurSens I think you can probably use the experimental adapter in the upstream zap repo: https://github.com/uber-go/zap/tree/master/exp

@ArthurSens
Copy link
Member Author

The changes made to the scrape manager are a bit more involved than I expected. We'll need a new zap->slog adapter to replace the existing zap->go-kit adapter.
Help appreciated because the solution was not immediately obvious to me 😬

@ArthurSens I think you can probably use the experimental adapter in the upstream zap repo: https://github.com/uber-go/zap/tree/master/exp

TIL, thanks for the tip! I'll try it out in the next days

@ArthurSens
Copy link
Member Author

This should be ready for review! cc @dashpole

And I'm assuming we don't need a changelog entry, is that right?

@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 10, 2025
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
@ArthurSens
Copy link
Member Author

I wonder if the compliance test failures are related to Prometheus changing the default NameValidationScheme 🤔

@dashpole
Copy link
Contributor

receivers::prometheus: unknown scrape protocol , supported: [OpenMetricsText0.0.1 OpenMetricsText1.0.0 PrometheusProto PrometheusText0.0.4 PrometheusText1.0.0]

Seems like the prometheus receivers' config validation changed?

@ArthurSens
Copy link
Member Author

ArthurSens commented Jan 10, 2025

receivers::prometheus: unknown scrape protocol , supported: [OpenMetricsText0.0.1 OpenMetricsText1.0.0 PrometheusProto PrometheusText0.0.4 PrometheusText1.0.0]

Seems like the prometheus receivers' config validation changed?

Ah, maybe be related to prometheus/prometheus#15136? The changelog says

[CHANGE] Scraping: Remove implicit fallback to the Prometheus text format in case of invalid/missing Content-Type and fail the scrape instead. Add ability to specify a `fallback_scrape_protocol` in the scrape config. #15136

@dashpole
Copy link
Contributor

yeah, we might need to set that in our default config

@ArthurSens
Copy link
Member Author

yeah, we might need to set that in our default config

The problem is that this is a per-scrape config, it looks like we can't just add that to global and that works for every scrape... or at least I'm missing some details somewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants