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

Setup instrumentation library #264

Merged
merged 2 commits into from
May 21, 2020
Merged

Setup instrumentation library #264

merged 2 commits into from
May 21, 2020

Conversation

dmathieu
Copy link
Member

Following @mwear's comment in #261, this is a proposal to implement the instrumentation library pattern, as I understand it from open-telemetry/oteps/pull/84.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Thanks for looking taking this on @dmathieu.

The instrumentation library should be the name and version of the instrumentation, not the underlying library. It really should be the name of the thing generating a span. Sorry for not explaining this better before. The reason for that, is the longer term goal is for OpenTelemetry to be built in to libraries, such as excon. The library itself would use excon and its version for spans it creates, while the instrumentation would identify itself as the excon adapter, and use the adapter version.

What you're doing in the PR is definitely on the right track. I can think we can do the following:

  • initialize the instrumentation library in Tracer using the name and version arguments, no need to set this explicitly from instrumentation
    • the name will be the adapter name
    • the version will be the adapter version
  • pipe the instrumentation library through to spans, and then span data as you are

Another note, is that the resource we're storing on the Tracer (with name and version) is not what we want longer term. I would leave it as is so @robertlaurin can use that to pipe through a process-level resource from #263.

@dmathieu
Copy link
Member Author

Thank you for the feedback @mwear. I've updated my PR to reflect your comment.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Thanks @dmathieu. This looks great!

I just wanted to double check with @robertlaurin. Will you be repurposing the library_resource to be just a resource in #263? If so, we should just leave it as is. If not, we should create an issue to remove it.

@mwear
Copy link
Member

mwear commented May 21, 2020

There could be changes on the horizon if this is accepted: open-telemetry/opentelemetry-proto#150. I'm merging this PR as it represents the current state of the specification.

@mwear mwear merged commit 2976399 into open-telemetry:master May 21, 2020
@dmathieu dmathieu deleted the instrumentation-library branch May 22, 2020 07:39
mwear added a commit that referenced this pull request May 26, 2020
See open-telemetry/opentelemetry-specification#447
Adapters will later be able to specify what instrumentation library they
are, and the exporters can then log this however they want: #264

Co-authored-by: Matthew Wear <[email protected]>
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.

2 participants