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.

Sign up for GitHub

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

Jump to bottom

Change TargetAllocator configmap for ScrapeConfig and Probe CR #3469

Merged
merged 11 commits into from
Dec 6, 2024
Merged

Change TargetAllocator configmap for ScrapeConfig and Probe CR #3469

merged 11 commits into from
Dec 6, 2024

Conversation

dominicqi
Copy link
Contributor

@dominicqi dominicqi commented Nov 18, 2024 •
edited
Loading

Description: Enable ScrapeConfigSelector and ProbeSelector support in OpenTelemetryTargetAllocatorPrometheusCR, create right configmap for target allocator

Link to tracking Issue(s):

Testing: add test in configmap_test.go

Documentation: ScrapeConfigSelector and ProbeSelector config in v1beta1 api doc

@dominicqi dominicqi requested a review from a team as a code owner November 18, 2024 06:07
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 18, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch 7 times, most recently from cc3c041 to 97b4885 Compare November 18, 2024 07:40
@dominicqi dominicqi changed the title Add TargetAllocator config for ScrapeConfig and Probe CR Change TargetAllocator configmap for ScrapeConfig and Probe CR Nov 18, 2024
swiatekm
swiatekm reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Looks good overall, some relatively minor nitpicks aside. Could you also add an e2e test for this behaviour? You can probably modify https://github.com/open-telemetry/opentelemetry-operator/tree/main/tests/e2e-targetallocator/targetallocator-prometheuscr.

Makefile Outdated Show resolved Hide resolved
apis/v1alpha1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved
apis/v1beta1/targetallocator_types.go Outdated Show resolved Hide resolved
apis/v1beta1/targetallocator_types.go Outdated Show resolved Hide resolved
@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch 2 times, most recently from 53614ed to d673153 Compare November 18, 2024 11:57
@dominicqi
Enable scrapeConfigSelector and probeSelector option in OpenTelemetry…
d87da69 
…TargetAllocatorPrometheusCR, create right configmap for target allocator

Signed-off-by: qiyang <[email protected]>
@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch from d673153 to d87da69 Compare November 18, 2024 12:01
@dominicqi
Copy link
Contributor Author

Looks good overall, some relatively minor nitpicks aside. Could you also add an e2e test for this behaviour? You can probably modify https://github.com/open-telemetry/opentelemetry-operator/tree/main/tests/e2e-targetallocator/targetallocator-prometheuscr.

I'm not very familiar with E2E testing yet. I will take some time to look into it.

@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch 3 times, most recently from d3861c1 to 4960f02 Compare November 19, 2024 07:58
@dominicqi
Copy link
Contributor Author

@swiatekm e2e test added

@swiatekm
Copy link
Contributor

@swiatekm e2e test added

Thanks for adding it! What you've added checks that setting the attribute on the CR leads to a change in the target allocator ConfgMap, which is a part of it. But since this PR effectively exposes this functionality to users, I'd like the new e2e test to also check that we actually allocate targets derived from ScrapeConfigs and Probes.

What I'd like you to add is the following:

  • Create a ScrapeConfig and/or Probe targetting the Otel collector you create in the test
  • Set the selector to select that ScrapeConfig/Probe
  • Use Jobs similar to the existing test to verify that the target allocator is actually allocating targets generated for the ScrapeConfig/Probe

Does that make sense?

For the record, the rest of your PR looks good to me.

@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch from 951c2a2 to 7ab7b88 Compare November 22, 2024 03:47
@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch from 5f4991f to 19720d8 Compare November 22, 2024 05:34
@jaronoff97
Copy link
Contributor

@dominicqi thanks for adding this :D I was planning on doing this when I got back from PTO but glad to see this is already here 🥳 With the changes that Mikolaj mentioned above this should be g2g.

@jaronoff97
Copy link
Contributor

looks like some test failures still, if you want to pair and figure them out or need a hand let me know!

@dominicqi
Copy link
Contributor Author

looks like some test failures still, if you want to pair and figure them out or need a hand let me know!

@jaronoff97 I'm a bit busy lately, it would be great if you could help ，thanks

@robertcoltheart
Copy link

Is this change needed to fully support ScrapeConfig resources? I was trying to figure out why my TA wasn't picking up the ScrapConfig resource with v0.114.0. Happy to contribute if so.

@jaronoff97
Copy link
Contributor

@robertcoltheart yes, that's the case. I've had some limited availibility... @dominicqi do you want me to push up some changes via a PR to your branch or do you want to pair?

@dominicqi
Copy link
Contributor Author

@robertcoltheart yes, that's the case. I've had some limited availibility... @dominicqi do you want me to push up some changes via a PR to your branch or do you want to pair?

I have some time now, I will try to fix it

@dominicqi
Merge remote-tracking branch 'refs/remotes/upstream/main' into scrape…
eefadde 
…_config_probe_selector

# Conflicts:
#	bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
#	bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml
@dominicqi
Merge remote-tracking branch 'origin/scrape_config_probe_selector' in…
2716ce4 
…to scrape_config_probe_selector
@dominicqi
Copy link
Contributor Author

dominicqi commented Dec 4, 2024 •
edited
Loading

all e2e passed @swiatekm

@dominicqi dominicqi force-pushed the scrape_config_probe_selector branch from df89324 to 1a6fdb9 Compare December 5, 2024 02:25
@swiatekm
Copy link
Contributor

swiatekm commented Dec 5, 2024

@jaronoff97 could you also have a look?

jaronoff97
jaronoff97 approved these changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

Perfect!! Thank you so much @dominicqi 🙇

@jaronoff97 jaronoff97 merged commit 438ffbc into open-telemetry:main Dec 6, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@jaronoff97 jaronoff97 jaronoff97 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[target allocator] Support the Probe and ScrapeConfig Prometheus Operator CRDs
4 participants
@dominicqi @swiatekm @jaronoff97 @robertcoltheart