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

[opampsupervisor] Enable emitting logs via the OTel sdk #38477

Merged
merged 17 commits into from
Mar 12, 2025

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Mar 8, 2025

Description

Experiment with enabling the logs SDK for the opampsupervisor telemetry

Link to tracking issue

Fixes

Testing

Updated unit test and tested manually. Here are the logs being exported to Honeycomb

image

Documentation

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth
@TylerHelmuth TylerHelmuth marked this pull request as ready for review March 10, 2025 17:47
@TylerHelmuth TylerHelmuth requested a review from a team as a code owner March 10, 2025 17:47
@TylerHelmuth TylerHelmuth requested a review from codeboten March 10, 2025 17:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Would it be feasible to add a simple E2E test to verify we get these logs in an OTLP sink?

@evan-bradley
Copy link
Contributor

On second thought, it would make more sense as a unit test since we're not starting a Collector binary, but it would be nice to have a smoke test to determine that setting this option has the intended effect.

@TylerHelmuth
Copy link
Member Author

On second thought, it would make more sense as a unit test since we're not starting a Collector binary, but it would be nice to have a smoke test to determine that setting this option has the intended effect.

@evan-bradley what did you have in mind besides the adding to the Test_NewSupervisorWithTelemetrySettings test? It would be good to have some e2e tests for the opamp supervisor but that feels out of scope for this PR.

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth
@evan-bradley
Copy link
Contributor

evan-bradley commented Mar 10, 2025

I think we should be able to use something in the testbed package like MockBackend to receive OTLP and check that we're getting in data.

We actually do have a pretty extensive E2E test suite in e2e_test.go at the root of the Supervisor directory. It's for tests that require a Collector binary to run. I was thinking we could maybe run tests for the Supervisor's telemetry without needing a Collector, but if we do then it's fine if it's an E2E test instead of a unit test.

@TylerHelmuth
Copy link
Member Author

I think we should be able to use something in the testbed package like MockBackend to receive OTLP and check that we're getting in data.

We'll need that for traces and metrics too. Do you want me to do that in this PR or in a followup?

@evan-bradley
Copy link
Contributor

Agreed, I forgot to bring it up in the other PRs. Let's just do logs in this PR and tackle traces and metrics in a follow-up.

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth
@TylerHelmuth
Copy link
Member Author

@evan-bradley test added

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
TylerHelmuth Tyler Helmuth

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@TylerHelmuth TylerHelmuth changed the title [opampsupervisor] Experiment with enabling logs sdk [opampsupervisor] Enable emitting logs via the OTel sdk Mar 11, 2025
@atoulme atoulme merged commit 1498c56 into open-telemetry:main Mar 12, 2025
172 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 12, 2025
@TylerHelmuth TylerHelmuth deleted the opampsupervisor-logs branch March 12, 2025 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants