Skip to content

Add instrumentation library resource to named tracers#632

Merged
bogdandrutu merged 3 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:named-tracers
Oct 31, 2019
Merged

Add instrumentation library resource to named tracers#632
bogdandrutu merged 3 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:named-tracers

Conversation

@arminru
Copy link
Copy Markdown
Member

@arminru arminru commented Oct 25, 2019 •
edited
Loading

This implements the instrumentation library resource for named tracers and makes it available to SpanProcessors (via ReadableSpan) and SpanExporters (via SpanData).

Resolves #617

@codecov-io

This comment has been minimized.

Sign in to view
jkwatson
jkwatson reviewed Oct 25, 2019
View reviewed changes
jkwatson
jkwatson reviewed Oct 25, 2019
View reviewed changes
jkwatson
jkwatson reviewed Oct 25, 2019
View reviewed changes
jkwatson
jkwatson approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

looks good! Just one issue to create, and it's good to go.

@jkwatson
Copy link
Copy Markdown
Contributor

jkwatson commented Oct 28, 2019

needs a rebase

bogdandrutu
bogdandrutu reviewed Oct 28, 2019
View reviewed changes
sdk/src/main/java/io/opentelemetry/sdk/trace/TracerSdk.java Outdated
this(Resource.getEmpty());
}

public TracerSdk(Resource instrumentationLibrary) {
Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu Oct 28, 2019

Choose a reason for hiding this comment

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

Why using the same Resource object. I think this will confuse users. What about having a InstrumentationLibraryInfo class with these infos?

Copy link
Copy Markdown
Member Author

@arminru arminru Oct 28, 2019

Choose a reason for hiding this comment

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

I was just sticking to the SDK spec which explicitly states a Resource has to be created, stored and returned. A POJO/AutoValue with properties would be fine with me as well, I'm not a fan of arbitrary key-value pairs for well-known keys anyway.
Would this require a spec change or is a deviation like that up to the implementation since it doesn't change any functionality?

Copy link
Copy Markdown
Member Author

@arminru arminru Oct 30, 2019

Choose a reason for hiding this comment

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

@bogdandrutu what do you say? Should we keep the Resource as defined by the OTEP and the SDK spec or modify it and go for a dedicated class?

Copy link
Copy Markdown
Contributor

@carlosalberto carlosalberto Oct 31, 2019

Choose a reason for hiding this comment

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

IHMO a dedicated class would be just fine ;)

Copy link
Copy Markdown
Member Author

@arminru arminru Oct 31, 2019

Choose a reason for hiding this comment

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

@bogdandrutu @carlosalberto - Alright, done!

@arminru arminru mentioned this pull request Oct 29, 2019
Closed
@bogdandrutu
Copy link
Copy Markdown
Member

bogdandrutu commented Oct 31, 2019

I would prefer a different class:

  • Resource describes the application/process
  • We can reuse that class for the internal map in the Factory and avoid the string concatenation.

@arminru arminru requested a review from bogdandrutu October 31, 2019 12:16
bogdandrutu
bogdandrutu reviewed Oct 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Small not, overall looks good

@bogdandrutu bogdandrutu merged commit 5e74548 into open-telemetry:master Oct 31, 2019
@arminru arminru deleted the named-tracers branch November 6, 2019 12:29
@arminru arminru mentioned this pull request Nov 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@carlosalberto carlosalberto carlosalberto approved these changes

@jkwatson jkwatson jkwatson approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@songy23 songy23 Awaiting requested review from songy23

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add library resources to named tracers in the SDK

5 participants

@arminru @codecov-io @jkwatson @bogdandrutu @carlosalberto