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] Split target allocator into internal package #33146

Closed
mx-psi opened this issue May 21, 2024 · 7 comments · Fixed by #33223
Closed

[receiver/prometheus] Split target allocator into internal package #33146

mx-psi opened this issue May 21, 2024 · 7 comments · Fixed by #33223
Labels

Comments

@mx-psi
Copy link
Member

mx-psi commented May 21, 2024

Component(s)

receiver/prometheus

Describe the issue you're reporting

The target allocator from the Prometheus receiver is a part of the codebase that not all contributors may be as familiar with as other parts. In the OpenTelemetry Operator, the target allocator is owned by a separate group of maintainers.

After discussion with the current set of codeowners of the receiver, this issue proposes splitting off this part of the codebase into an internal package (or a separate module under the top-level internal/ folder if there is a possibility of reuse), so that we can find new owners for this part of the codebase and ensure we can appropriately handle PRs and issues related to this part of the code.

cc @dashpole @Aneurysm9

@mx-psi mx-psi added the priority:p2 Medium label May 21, 2024
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label May 21, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@dashpole
Copy link
Contributor

I support this change

@swiatekm
Copy link
Contributor

I agree in spirit. The target allocator integration should be owned by the codeowners of the target allocator itself, which in principle includes some combination of @open-telemetry/operator-ta-maintainers and @open-telemetry/operator-maintainers.

I don't know if I like the idea of making it a separate package. It would only have a single consumer, with no real prospects of having more in the future, and would probably be heavily coupled to the prometheus receiver internals anyway. Do we have the option of moving to to a separate Go file, and having different codeowners just for that file?

@mx-psi
Copy link
Member Author

mx-psi commented May 23, 2024

I don't know if I like the idea of making it a separate package. It would only have a single consumer, with no real prospects of having more in the future, and would probably be heavily coupled to the prometheus receiver internals anyway. Do we have the option of moving to to a separate Go file, and having different codeowners just for that file?

A separate package could just be a receiver/prometheusreceiver/internal/targetallocator folder, no need for it to be exposed to other modules if there's a single consumer. A single file is also fine (although I suppose at least two files are needed, one of them for testing)

@dashpole
Copy link
Contributor

In my current PoC, it is 2 files + 2 test files, with the configuration being split into a config.go file. I'll push what I have tomorrow if I can

@dashpole
Copy link
Contributor

#33223

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jul 24, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this issue Sep 12, 2024
okankoAMZ pushed a commit to amazon-contributing/opentelemetry-collector-contrib that referenced this issue Oct 11, 2024
okankoAMZ added a commit to amazon-contributing/opentelemetry-collector-contrib that referenced this issue Oct 14, 2024
* Split target allocator into an internal package (open-telemetry#33223)

Fixes
open-telemetry#33146

* basic working copy

* Added k8 test back

---------

Co-authored-by: David Ashpole <dashpole@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants