Skip to content

[receiver/prometheus] Add fallback_scrape_protocol when using target allocator #39672

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

Merged
merged 5 commits into from
May 9, 2025

Conversation

tinou98
Copy link
Contributor

@tinou98 tinou98 commented Apr 26, 2025

This applies a similar fix as 72b834b, when using TargetAllocator.

Description

For the same reason as we already add a fallback_scrape_protocol to the config, this PR add the same behavior when using the Target Allocator.

It's important for me, as even the fallbackScrapeProtocol from the Prometheus CRD is ignored (I guess it's because the allocator advertise itself as version 2.55.1, which does not support (nor require) fallback_scrape_protocol).

Maybe it's a change that should be done on the target-allocator side, but I kind of like the symmetry with what we're already doing with provided configuration.

Link to tracking issue

Same issue as #37902, but when using a target allocator

…allocator

This applies a similar fix as 72b834b,
but when using TargetAllocator.

Signed-off-by: MATILLAT Quentin <[email protected]>
@tinou98 tinou98 requested review from dashpole, ArthurSens and a team as code owners April 26, 2025 19:05
Copy link

linux-foundation-easycla bot commented Apr 26, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Apr 26, 2025
@github-actions github-actions bot requested review from Aneurysm9 and krajorama April 26, 2025 19:06
@atoulme
Copy link
Contributor

atoulme commented Apr 26, 2025

Is there a way to test this? Please add a changelog.

tinou98 added 2 commits April 27, 2025 09:56
…l addition when using target allocator

Signed-off-by: MATILLAT Quentin <[email protected]>
This tests that the provided value is properlly forwared, and if there
is no value, that a default is applied

Signed-off-by: MATILLAT Quentin <[email protected]>
@tinou98
Copy link
Contributor Author

tinou98 commented Apr 27, 2025

I created the changelog entry, and added a test.
But it seems that the TestTargetAllocatorJobRetrieval test was already a victim of data races : we wait for the download of all the config (with allocator.wg.Wait()), but this does not guarantee that the config is updated. So if it fails on TestTargetAllocatorJobRetrieval/update_metric_relabel_config_regex this might just be bad luck.

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, approved with nit comments.

@ArthurSens
Copy link
Member

Hmm, looks like we're getting blocked by a vulnerability in another component. I'm going to rebase the PR on top of main to see if it get's solved

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

It worked!

@ArthurSens ArthurSens added ready to merge Code review completed; ready to merge by maintainers and removed waiting-for-code-owners labels May 8, 2025
@atoulme atoulme merged commit 7333a37 into open-telemetry:main May 9, 2025
189 of 190 checks passed
@github-actions github-actions bot added this to the next release milestone May 9, 2025
dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this pull request May 23, 2025
…allocator (open-telemetry#39672)

This applies a similar fix as 72b834b,
when using TargetAllocator.

#### Description
For the same reason as we already add a `fallback_scrape_protocol` to
the config, this PR add the same behavior when using the Target
Allocator.

It's important for me, as even the `fallbackScrapeProtocol` from the
Prometheus CRD is ignored (I guess it's because the allocator advertise
itself as version 2.55.1, which does not support (nor require)
`fallback_scrape_protocol`).

Maybe it's a change that should be done on the target-allocator side,
but I kind of like the symmetry with what we're already doing with
provided configuration.

#### Link to tracking issue
Same issue as open-telemetry#37902, but when using a target allocator

---------

Signed-off-by: MATILLAT Quentin <[email protected]>
Co-authored-by: George Krajcsovits <[email protected]>
Co-authored-by: Arthur Silva Sens <[email protected]>
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.

4 participants