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

chore: use interface for context types #1515

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Sep 10, 2020

In most cases, our current code base is using the concrete Context class as a type. This makes for incompatibilities when differing versions or subclasses of elements using context are used. For instance, it is possible to create a TracerProvider which satisfies all of the public methods, but typescript complains when you try to register it with the global API because private methods mismatch, even though they aren't used by the API.

This changes the context-base module to export an interface, and root context which is declared to have the type of the interface, not the concrete class. This also changes usages of Context throughout the project to use the interface and the new exported root context.

Overall, this enhances the separation of the API module and its interfaces from the concrete implementations of the SDK and context.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #1515 into master will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   93.82%   93.82%   -0.01%     
==========================================
  Files         154      154              
  Lines        4763     4761       -2     
  Branches      952      952              
==========================================
- Hits         4469     4467       -2     
  Misses        294      294              
Impacted Files Coverage Δ
packages/opentelemetry-api/src/api/propagation.ts 57.57% <0.00%> (ø)
...ackages/opentelemetry-shim-opentracing/src/shim.ts 88.00% <ø> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <75.00%> (-0.19%) ⬇️
packages/opentelemetry-context-base/src/context.ts 76.47% <85.71%> (-1.31%) ⬇️
...ontext-async-hooks/src/AsyncHooksContextManager.ts 100.00% <100.00%> (ø)
...async-hooks/src/AsyncLocalStorageContextManager.ts 100.00% <100.00%> (ø)
...entelemetry-context-base/src/NoopContextManager.ts 100.00% <100.00%> (ø)
packages/opentelemetry-core/src/context/context.ts 95.83% <100.00%> (ø)
...metry-core/src/context/propagation/B3Propagator.ts 100.00% <100.00%> (ø)
...ore/src/correlation-context/correlation-context.ts 100.00% <100.00%> (ø)
... and 1 more

@dyladan
Copy link
Member Author

dyladan commented Sep 11, 2020

@xirzec we recently had a brief conversation about the idea that concrete classes have discrepancies more often than interfaces. Curious to know what you think of this change since my intention is to alleviate issues similar to what we discussed then.

@xirzec
Copy link

xirzec commented Sep 12, 2020

@dyladan Yes, the issue you're solving with this PR is the exact reason I've started preferring interfaces whenever possible. Nice work!

@dyladan
Copy link
Member Author

dyladan commented Sep 15, 2020

@open-telemetry/javascript-approvers would like to get this merged before I leave for vacation :)

@dyladan
Copy link
Member Author

dyladan commented Sep 15, 2020

@dyladan Yes, the issue you're solving with this PR is the exact reason I've started preferring interfaces whenever possible. Nice work!

OK good. Just wanted to make sure I understood fully the issue you were trying to solve. I believe we will also have the same problem with TraceFlags, which is currently an enum in the API. Aside from the fact that enum is an incorrect type for this, it also causes typescript to fail if you bring your own tracer which redefines this enum.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, one concern with keeping the static

packages/opentelemetry-context-base/src/index.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/index.ts Outdated Show resolved Hide resolved
@dyladan
Copy link
Member Author

dyladan commented Sep 21, 2020

@obecny fixed PTAL

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan merged commit 75ae34c into open-telemetry:master Sep 21, 2020
@dyladan dyladan deleted the context-interface branch September 21, 2020 19:14
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants