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

Update instrumentations to use tracer_provider for creating tracer if given, otherwise use global tracer provider #402

Merged
merged 45 commits into from
Apr 28, 2021

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Apr 4, 2021

Description

An optional tracer provider can be passed to instrumentations to use for creating tracer instances. There is no consistency in instrumentations.

Contributes to #401

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Thanks. Minor comment. Looks good otherwise.

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Didn't realize this was a draft. Will review again once final version is available.

@srikanthccv srikanthccv marked this pull request as ready for review April 10, 2021 01:02
@srikanthccv srikanthccv requested a review from a team as a code owner April 10, 2021 01:02
@srikanthccv srikanthccv requested review from owais and aabmass and removed request for a team April 10, 2021 01:02
@lzchen
Copy link
Contributor

lzchen commented Apr 13, 2021

@owais @lonewolf3739
Thoughts on this?

@srikanthccv
Copy link
Member Author

@owais @lonewolf3739
Thoughts on this?

I believe the tracer_provider is used for resource attributes. Adding to your point here is the small discussion we had about using the resource from span instead #345 (comment). If there is no other reason to pass tracer provider I think we can get rid of it.

@owais
Copy link
Contributor

owais commented Apr 13, 2021

@lonewolf3739 @lzchen regarding logging instrumentation, it would be ideal if we could update it to extract service name from the span instead of provider but I don't know if that is possible without adding a dependency on the SDK.

One possible solution could be to implement log injection as a feature of the SDK instead of an instrumentation. Technically it's not really an instrumentation as it doesn't instrument the logging module (doesn't trace it) but provides utility to augment generated logs with additional data. The behavior is actually part of upcoming Otel log spec as well.

@lzchen
Copy link
Contributor

lzchen commented Apr 13, 2021

@owais
Isn't Resource an SDK only concept anyways? I feel the logging enriching feature is not an instrumentation, and needs to depend on the SDK.

@owais
Copy link
Contributor

owais commented Apr 14, 2021

Isn't Resource an SDK only concept anyways? I feel the logging enriching feature is not an instrumentation, and needs to depend on the SDK.

I agree. We can add a dependency on the SDK to begin with and later move the package out of instrumentations into either it's own package or into the SDK.

@lzchen
Copy link
Contributor

lzchen commented Apr 26, 2021

@owais
Can you review once more?

@codeboten codeboten merged commit 3ec7736 into open-telemetry:main Apr 28, 2021
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.

None yet

4 participants