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] include scrape configs provided via scrape_config_files #34897

Merged
merged 12 commits into from
Sep 11, 2024

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Aug 28, 2024

Description: This PR fixes a bug in the prometheus receiver where the scrape configs provided via scrape_config_files were not applied. Using scrape_config_files instead of providing the scrape configs directly does come with some limitations regarding the use of env vars, as also mentioned in #34786 (comment).

Link to tracking Issue: #34786

Testing: Added unit tests

… up receiver

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Aug 28, 2024
@bacherfl bacherfl changed the title Fix/34786/scrape config file [receiver/prometheus] include scrape configs provided via scrape_config_files Aug 28, 2024
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
if !enableNativeHistogramsGate.IsEnabled() {
// Enforce scraping classic histograms to avoid dropping them.
for _, scrapeConfig := range cfg.ScrapeConfigs {
for _, scrapeConfig := range scrapeConfigs {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have the limitation that the scrapeConfig.ScrapeClassigHistograms property is only applied to the scrapeconfigs provided directly in the config, but not the ones via scrape_config_files, as the scrapeManager internally calls GetScrapeConfigs(), which leads to the scrapeConfigs being read from the file system again.
One potential solution could be to set cfg.ScrapeConfigs here to the result of GetScrapeConfigs(), and clear the ScrapeConfigFiles property, but this feels a bit hacky. What do you think @dashpole ?

# Conflicts:
#	receiver/prometheusreceiver/config.go
#	receiver/prometheusreceiver/metrics_receiver.go
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review September 3, 2024 07:14
@bacherfl bacherfl requested a review from a team September 3, 2024 07:14
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

bacherfl and others added 2 commits September 5, 2024 07:11
Co-authored-by: Antoine Toulme <antoine@toulme.name>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Sep 9, 2024
@mx-psi mx-psi merged commit c839ca6 into open-telemetry:main Sep 11, 2024
155 of 156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheus Prometheus receiver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants