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

replace InstrumentationLibrary with InstrumentationScope #405

Merged
merged 3 commits into from
Jun 12, 2022

Conversation

tsloughter
Copy link
Member

Spec change can be found in PR:
open-telemetry/opentelemetry-specification#2276

@tsloughter
Copy link
Member Author

This is mostly ready but need to find where it isn't backwards compatible and fix those spots.

I think the only place it isn't is that it would be getting rid of the instrumentation_library creation functions in opentelemetry module. I guess we just add those back and have them return scopes instead.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #405 (7dadcc2) into main (ccbe7f1) will decrease coverage by 0.04%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
- Coverage   73.23%   73.18%   -0.05%     
==========================================
  Files          53       53              
  Lines        1655     1656       +1     
==========================================
  Hits         1212     1212              
- Misses        443      444       +1     
Flag Coverage Δ
api 68.59% <50.00%> (-0.12%) ⬇️
elixir 17.52% <0.00%> (-0.03%) ⬇️
erlang 74.60% <83.33%> (-0.05%) ⬇️
exporter 73.59% <85.71%> (ø)
sdk 78.09% <100.00%> (ø)
zipkin 53.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
apps/opentelemetry/src/otel_batch_processor.erl 70.10% <ø> (ø)
apps/opentelemetry/src/otel_simple_processor.erl 70.37% <ø> (ø)
apps/opentelemetry_api/src/opentelemetry.erl 80.00% <50.00%> (-0.90%) ⬇️
...ntelemetry_exporter/src/opentelemetry_exporter.erl 73.59% <85.71%> (ø)
apps/opentelemetry/src/otel_span_ets.erl 72.09% <100.00%> (ø)
apps/opentelemetry/src/otel_tracer_default.erl 100.00% <100.00%> (ø)
apps/opentelemetry/src/otel_tracer_server.erl 71.05% <100.00%> (ø)

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 ccbe7f1...7dadcc2. Read the comment docs.

@tsloughter
Copy link
Member Author

Hmm, nevermind, it looks like we only have get_tracer/3 instead of a get_tracer that accepted an InstrumentationLibrary. So no user would be using opentelemetry:instrumentation_library. We may not want to bother keeping the function instrumentation_library in the module as it could be confusing and shouldn't be being used.

@tsloughter tsloughter force-pushed the instrumentation-scope branch from 2569f17 to 597e0ee Compare June 8, 2022 21:26
@@ -86,7 +87,7 @@

-type tracer() :: {module(), term()}.

-type instrumentation_library() :: #instrumentation_library{}.
-type instrumentation_scope() :: #instrumentation_scope{}.
Copy link
Member

Choose a reason for hiding this comment

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

is it worth maintaining the type alias as well?

-type instrumentation_library() :: #instrumentation_scope{}. or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt it. If any user actually uses the library record the least of what they have to deal with is the type spec.

CHANGELOG.md Outdated Show resolved Hide resolved
@tsloughter tsloughter force-pushed the instrumentation-scope branch from 597e0ee to 7dadcc2 Compare June 11, 2022 18:56
@tsloughter tsloughter requested a review from ferd June 11, 2022 18:56
@tsloughter tsloughter merged commit 2964d0b into open-telemetry:main Jun 12, 2022
@tsloughter tsloughter deleted the instrumentation-scope branch June 12, 2022 12:00
@tsloughter tsloughter mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants