Skip to content

Conversation

axw
Copy link
Contributor

@axw axw commented Aug 5, 2025

Description

Move the telemetry implementation from service/telemetry to service/telemetry/otelconftelemetry, with the exception of the Settings type which is intended to remain in the service/telemetry package permanently. The goal here is to separate the interface (which is to be defined in a followup) and implementation, so multiple implementations can exist.

This move is in preparation for creating a new Telemetry and Factory interface, similar to pipeline components. In the end, the implementation (e.g. otelconftelemetry) will be completely self contained, providing a NewFactory function to obtain a factory that can be used to construct a Telemetry implementation.

The next steps after this PR would be:

  1. Update https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7bf3615b56d443c522d5b5591ce53d5fe43b8b47/cmd/opampsupervisor/supervisor/config/config.go#L25 to use otelconftelemetry. Eventually the opampsupervisor should probably use the Factory & Telemetry, but that can be done later on.
  2. Create the Factory and Telemetry interface in service/telemetry, and update otelconftelemetry to implement it, encapsulating the creation of the SDK & SDK resource. We would continue directly constructing the otelconftelemetry factory to keep the change contained. At this stage we could close Move telemetry initialization logic to service/telemetry #8170.
  3. Update otelcol and service packages to inject the telemetry factory & default config into the service's settings and default config, and remove direct references to otelconftelemetry in the service package. At this stage we could close Allow overriding telemetry providers #4970.

Link to tracking issue

Part of #4970

Testing

N/A, non-functional change.

Documentation

N/A

@axw axw force-pushed the otelconftelemetry branch 2 times, most recently from 4c8305d to 974d339 Compare August 5, 2025 07:41
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.23%. Comparing base (e8497df) to head (9b1b8d8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13560      +/-   ##
==========================================
- Coverage   92.24%   92.23%   -0.02%     
==========================================
  Files         605      605              
  Lines       31909    31911       +2     
==========================================
- Hits        29435    29433       -2     
- Misses       1952     1955       +3     
- Partials      522      523       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@axw axw force-pushed the otelconftelemetry branch from 974d339 to 631001d Compare August 5, 2025 08:37
Move the implementation to service/telemetry/otelconftelemetry.
For now I've moved everything except for Settings. I'll introduce
a new Factory and Telemetry interface in a followup, and remove
the Factory interface from otelconftelemetry.
@axw axw force-pushed the otelconftelemetry branch from 631001d to 8ba1765 Compare August 5, 2025 08:57
@axw axw marked this pull request as ready for review August 5, 2025 09:37
@axw axw requested a review from a team as a code owner August 5, 2025 09:37
@axw axw requested a review from TylerHelmuth August 5, 2025 09:37
// NewFactory creates a new Factory.
func NewFactory() Factory {
//
// TODO remove the sdk and res parameters once the factory is fully
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a reference to an issue tracking this here and in the other TODO?

I would also add a proper godoc comment that says "this is experimental and will change soon use at your own risk"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've linked to #4970, and added some NOTEs to warn against use in the mean time.

@axw axw requested a review from mx-psi August 8, 2025 01:32
@mx-psi mx-psi added this pull request to the merge queue Aug 8, 2025
Merged via the queue into open-telemetry:main with commit ed09fb9 Aug 8, 2025
57 checks passed
@axw axw deleted the otelconftelemetry branch August 8, 2025 08:23
csatib02 added a commit to kube-logging/telemetry-controller that referenced this pull request Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow overriding telemetry providers Move telemetry initialization logic to service/telemetry

3 participants