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

config: NewSDK can return valid TracerProvider #4741

Merged

Conversation

codeboten
Copy link
Contributor

This PR is another attempt at filling in the SDK details for NewSDK. It breaks the #4617 per signal to make it more manageable to review.

Part of #4371

This PR is another attempt at filling in the SDK details for NewSDK. It breaks the open-telemetry#4617 per signal to make it more manageable to review.

Part of open-telemetry#4371

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten requested a review from a team December 21, 2023 20:12
Alex Boten added 2 commits December 21, 2023 12:13
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

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

Comparison is base (3cecdcf) 81.1% compared to head (f09ea01) 81.2%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4741     +/-   ##
=======================================
+ Coverage   81.1%   81.2%   +0.1%     
=======================================
  Files        150     150             
  Lines      10753   10900    +147     
=======================================
+ Hits        8725    8858    +133     
- Misses      1872    1882     +10     
- Partials     156     160      +4     
Files Coverage Δ
config/trace.go 97.1% <97.1%> (-2.9%) ⬇️
config/config.go 79.5% <37.5%> (-20.5%) ⬇️

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Side note: I have not managed to find time to make a complete review, but I think it is worth to give some feedback.

config/config_test.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
codeboten and others added 2 commits December 30, 2023 10:13
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Outdated Show resolved Hide resolved
config/trace_test.go Outdated Show resolved Hide resolved
Alex Boten added 2 commits January 10, 2024 12:11
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten force-pushed the codeboten/add-tracer-provider-sdk branch from 555d28d to f8c02e9 Compare January 10, 2024 20:12
codeboten and others added 6 commits January 10, 2024 12:13
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
config/trace.go Outdated Show resolved Hide resolved
config/trace.go Show resolved Hide resolved
config/trace.go Show resolved Hide resolved
Alex Boten added 4 commits January 10, 2024 15:37
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
config/trace.go Outdated Show resolved Hide resolved
@pellared pellared changed the title implement TracerProvider functionality config: NewSDK implement TracerProvider functionality Jan 11, 2024
@pellared pellared changed the title config: NewSDK implement TracerProvider functionality config: NewSDK can return valid TracerProvider Jan 11, 2024
codeboten pushed a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Jan 11, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <aboten@lightstep.com>
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I just have one comment before merging.

Comment on lines +123 to +130
func newResource(res *Resource) (*resource.Resource, error) {
if res == nil {
return resource.Default(), nil
}
return resource.Merge(resource.Default(),
resource.NewWithAttributes(semconv.SchemaURL,
semconv.ServiceName(*res.Attributes.ServiceName),
))
Copy link
Member

Choose a reason for hiding this comment

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

How about moving to resource.go and adding tests in resource_test.go?

Can we do it in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll move this next before the meter provider PR

Copy link
Member

@pellared pellared Jan 16, 2024

Choose a reason for hiding this comment

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

I created #4821

@pellared pellared merged commit 0a02c5b into open-telemetry:main Jan 16, 2024
22 checks passed
@codeboten codeboten deleted the codeboten/add-tracer-provider-sdk branch January 16, 2024 17:53
codeboten pushed a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Jan 16, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Jan 22, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Mar 4, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Mar 4, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Apr 12, 2024
Follow up to open-telemetry#4741, does the same but for metric signal.

Fixes open-telemetry#4371

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
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