Skip to content

Conversation

@axw
Copy link
Contributor

@axw axw commented Sep 15, 2025

Description

The service.Settings type now has a TelemetryFactory field for injecting the telemetry.Factory to be used for creating a logger and logger provider, meter provider, and tracer provider. The otelcol package is hard-coded to inject an otelconftelemetry factory for now. In a followup we will make it possible to inject the telemetry through otelcol.Factories.

Link to tracking issue

Part of #4970

Testing

Manually verified that service telemetry configuration still works with a collector binary.

Documentation

N/A

@axw axw force-pushed the service-settings-telemetryfactory branch 3 times, most recently from 902c8dc to 875114a Compare September 15, 2025 01:28
@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.66%. Comparing base (2e5abce) to head (3e1f81e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13838      +/-   ##
==========================================
- Coverage   91.67%   91.66%   -0.01%     
==========================================
  Files         652      653       +1     
  Lines       42516    42550      +34     
==========================================
+ Hits        38978    39005      +27     
- Misses       2730     2736       +6     
- Partials      808      809       +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 service-settings-telemetryfactory branch from 875114a to 85d7247 Compare September 15, 2025 02:07
The `service.Settings` type now has a `TelemetryFactory`
field for injecting the `telemetry.Factory` to be used
for creating a logger and logger provider, meter provider,
and tracer provider.

The `otelcol` package is hard-coded to inject an
otelconftelemetry factory for now. In a followup we will
make it possible to inject the telemetry through
`otelcol.Factories`.
@axw axw force-pushed the service-settings-telemetryfactory branch from 85d7247 to 872b902 Compare September 15, 2025 02:41
@axw axw marked this pull request as ready for review September 15, 2025 03:23
@axw axw requested a review from a team as a code owner September 15, 2025 03:23
@axw axw requested a review from evan-bradley September 15, 2025 03:23
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: I've put this down as a breaking API change because the new setting is required.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Before moving forward, I really want to first fix #4970 (comment)

@axw axw requested a review from bogdandrutu September 24, 2025 09:48
@bogdandrutu bogdandrutu dismissed their stale review September 25, 2025 04:09

Concerns fixe

@axw axw marked this pull request as draft October 7, 2025 05:36
@axw axw marked this pull request as ready for review October 7, 2025 07:55
@axw axw requested a review from dmitryax as a code owner October 7, 2025 07:55
@axw axw requested a review from mx-psi October 7, 2025 07:55
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

This looks good to me. I am curious as to what the error message is if you don't provide a TelemetryFactory. Maybe it is worth to provide a more detailed error message

@axw
Copy link
Contributor Author

axw commented Oct 8, 2025

@mx-psi service.New it would have panicked, but I've changed it to check and return an error: "telemetry factory not provided".

@mx-psi mx-psi added this pull request to the merge queue Oct 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2025
A new release was done and this makes the CI fail if I don't change it
@mx-psi mx-psi enabled auto-merge October 8, 2025 08:44
@mx-psi mx-psi added this pull request to the merge queue Oct 8, 2025
Merged via the queue into open-telemetry:main with commit 01b989f Oct 8, 2025
60 of 61 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 8, 2025
@axw axw deleted the service-settings-telemetryfactory branch October 8, 2025 09:27
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.

3 participants