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

[service] remove code to configure otel SDK #9131

Closed
wants to merge 6 commits into from
Closed

[service] remove code to configure otel SDK #9131

wants to merge 6 commits into from

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Dec 18, 2023 •
edited

This code will live in the otel-go-contrib config package instead. This PR depends on open-telemetry/opentelemetry-go-contrib#4617

Leaving this as draft until config package is updated. Ideally I'd like to get the MeterProvider configured in this PR as well

Addressing only TracerProvider in this initial PR.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 3, 2024
@codeboten codeboten removed the Stale label Jan 4, 2024
@codeboten codeboten marked this pull request as ready for review January 17, 2024 00:13
@codeboten codeboten requested a review from a team as a code owner January 17, 2024 00:13
@codeboten codeboten requested a review from mx-psi January 17, 2024 00:13
@codecov Codecov
Copy link

codecov bot commented Jan 17, 2024 •
edited

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (72f6ce1) 90.34% compared to head (4353115) 90.34%.
Report is 64 commits behind head on main.

❗ Current head 4353115 differs from pull request most recent head 51f6ed6. Consider uploading reports for the commit 51f6ed6 to get more accurate results

Files Patch % Lines
service/telemetry.go 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9131      +/-   ##
==========================================
- Coverage   90.34%   90.34%   -0.01%     
==========================================
  Files         340      340              
  Lines       17989    17873     -116     
==========================================
- Hits        16253    16148     -105     
+ Misses       1412     1404       -8     
+ Partials      324      321       -3

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

mx-psi
mx-psi approved these changes Jan 17, 2024
View reviewed changes
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.

Should we wait for the next config release?

@codeboten
Copy link
Contributor Author

Should we wait for the next config release?

I don't have a strong preference here, happy to go either ways

Alex Boten added 3 commits January 17, 2024 14:09
[service] remove code to configure otel SDK
a814395 
This code will live in the otel-go-contrib `config` package instead. This PR depends on open-telemetry/opentelemetry-go-contrib#4617

Signed-off-by: Alex Boten <aboten@lightstep.com>
update dep
377a824 
Signed-off-by: Alex Boten <aboten@lightstep.com>
set processors
34f586a 
Signed-off-by: Alex Boten <aboten@lightstep.com>
@mx-psi
Copy link
Member

mx-psi commented Jan 18, 2024

Should we wait for the next config release?

I don't have a strong preference here, happy to go either ways

They may have to do a release anyway for the gRPC issue, so I would hold on merging this for a bit to see if we get a release for this module as well.

mx-psi
mx-psi approved these changes Jan 19, 2024
View reviewed changes
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.

LGTM, I merged main and bumped to v0.2.0 which has the changes we need for this

@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 19, 2024
mx-psi
mx-psi reviewed Jan 19, 2024
View reviewed changes
service/telemetry.go
@@ -75,13 +74,20 @@ func (tel *telemetryInitializer) init(res *resource.Resource, settings servicete
}

settings.Logger.Info("Setting up own telemetry...")
configuredSDK, err := config.NewSDK(
Copy link
Member

Choose a reason for hiding this comment

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

One question: are we not passing the resource anymore here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. I'm working on an issue in the go-contrib repo where setting resource attributes via config is currently restricted to service name, as the go-jsonschema library doesn't have support for additionalProperties on an object that has other properties specified.

Putting this back into draft until that's resolved (open-telemetry/opentelemetry-go-contrib#4828)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mx-psi here's roughly what i think the change will look like with a resource: 51f6ed6

the code isn't in the config package yet though, so it will likely change

Copy link
Member

Choose a reason for hiding this comment

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

👍 Makes sense, let's wait on that issue to be solved then :)

@mx-psi mx-psi mentioned this pull request Jan 19, 2024
Closed
@codeboten codeboten marked this pull request as draft January 19, 2024 14:25
set resource in config
51f6ed6 
Signed-off-by: Alex Boten <aboten@lightstep.com>
@mx-psi mx-psi mentioned this pull request Jan 19, 2024
Merged
codeboten pushed a commit that referenced this pull request Jan 22, 2024
@mx-psi
[service] Refactor telemetry initialization (#9320)
4d73b4f 
A part of #9319 that can be done without waiting for #9131
TylerHelmuth pushed a commit to TylerHelmuth/opentelemetry-collector that referenced this pull request Jan 23, 2024
@mx-psi @TylerHelmuth
[service] Refactor telemetry initialization (open-telemetry#9320)
14b7986 
A part of open-telemetry#9319 that can be done without waiting for open-telemetry#9131
@mx-psi mx-psi mentioned this pull request Jan 24, 2024
Merged
codeboten pushed a commit that referenced this pull request Jan 30, 2024
@mx-psi
[service] Move TracerProvider initialization to service/telemetry pac…
07a00ff 
…kage (#9384)

Second redo of
#8171 that
does not depend on
#9131

Link to tracking Issue: Updates
#8170

---------

Co-authored-by: Alex Boten <aboten@lightstep.com>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mx-psi mx-psi mx-psi approved these changes

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@codeboten @mx-psi