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

Make OpenTelemetrySdk implement the OpenTelemetry interface and have SPI find it. #1857

Merged

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Oct 22, 2020

See #1737 for the discussion.

I don't want to merge a bad PR and a followup renaming like last time since it's confusing and a bit too tedious. I have two commits here, 949bfec is the significant one to check out, the other just applies renames.

Fixes #1109 (each app can have its own OpenTelemetry for different Resource which seems to be the crux of the issue)
Fixes #1149
Fixes #1454
Fixes #747

@anuraaga
Copy link
Contributor Author

John has suggested using DefaultOpenTelemetry within the SDK, or other, implementations of the API. I think it's a good idea but requires making it public - I'd like to address that in a separate PR.

@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@23444d6). Click here to learn what that means.
The diff coverage is 77.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1857   +/-   ##
=========================================
  Coverage          ?   85.56%           
  Complexity        ?     1461           
=========================================
  Files             ?      176           
  Lines             ?     5679           
  Branches          ?      593           
=========================================
  Hits              ?     4859           
  Misses            ?      616           
  Partials          ?      204           
Impacted Files Coverage Δ Complexity Δ
...try/exporters/inmemory/InMemoryMetricExporter.java 100.00% <ø> (ø) 8.00 <0.00> (?)
.../main/java/io/opentelemetry/SendTraceToJaeger.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 75.28% <75.28%> (ø) 13.00 <13.00> (?)
...in/java/io/opentelemetry/DefaultOpenTelemetry.java 98.36% <100.00%> (ø) 17.00 <1.00> (?)
.../src/main/java/io/opentelemetry/OpenTelemetry.java 100.00% <100.00%> (ø) 14.00 <2.00> (?)
.../io/opentelemetry/sdk/OpenTelemetrySdkFactory.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...n/java/io/opentelemetry/sdk/trace/SpanWrapper.java 100.00% <0.00%> (ø) 22.00% <0.00%> (?%)
...n/java/io/opentelemetry/sdk/trace/StringUtils.java 77.77% <0.00%> (ø) 8.00% <0.00%> (?%)
...in/java/io/opentelemetry/sdk/metrics/MeterSdk.java 100.00% <0.00%> (ø) 18.00% <0.00%> (?%)
...ava/io/opentelemetry/sdk/metrics/ViewRegistry.java 63.63% <0.00%> (ø) 6.00% <0.00%> (?%)
... and 172 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23444d6...980644f. Read the comment docs.

@Oberon00
Copy link
Member

Oberon00 commented Oct 22, 2020

Have you considered #1834? How would we solve this after this PR? (I did not think about it in depth yet -- maybe it gets easier)

@anuraaga
Copy link
Contributor Author

Added some comments in the issue - I am thinking that regardless we do want to have a single entry point into the SDK. It's the very common use case so presenting the single entry point simplifies the experience. But we may be able to improve the ux for custom sdk users by adding similar entry points into the SDK components. I don't believe that this change would hinder such improvements and want to make those with the understanding that we do want to optimize for the common case of full SDK if we ever make a trade-off.

QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

Thanks! This needs a few cleanups, but it's looking pretty solid to me.

@anuraaga anuraaga force-pushed the opentelemetry-injectable-part2 branch from 8e3229d to 980644f Compare October 23, 2020 04:09
Copy link
Contributor Author

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Reverted QUICKSTART change since we'll do that with the release - copied it to https://gist.github.com/anuraaga/e53f5c226624b6efd83ceb276438ebd4

QUICKSTART.md Outdated Show resolved Hide resolved
QUICKSTART.md Outdated Show resolved Hide resolved
Copy link
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. Thanks!

@jkwatson jkwatson merged commit 934c9ed into open-telemetry:master Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants