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: clean up API for release #1759

Closed

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Dec 17, 2020

Summary

  • Do not export Noop* classes
    • These were only depended on by tests in other packages. For now, I've made them import from the deep direct path like import {} from '@opentelemetry/api/build/src/folder/file'; but in the future, other package tests should probably not depend on these internal details of the API if possible.
  • Do not export NOOP_* constants
  • Do not export INVALID_* SpanContext constants
  • Rename api.StatusCode to api.SpanStatusCode
  • Move createContextKey to context.createKey
  • Rename api.EntryValue to api.BaggageEntryValue
  • Rename api.EntryTtl to api.BaggageEntryTtl

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #1759 (9357fec) into master (7e5da41) will decrease coverage by 0.20%.
The diff coverage is 77.52%.

@@            Coverage Diff             @@
##           master    #1759      +/-   ##
==========================================
- Coverage   92.07%   91.87%   -0.21%     
==========================================
  Files         166      166              
  Lines        5593     5609      +16     
  Branches     1199     1201       +2     
==========================================
+ Hits         5150     5153       +3     
- Misses        443      456      +13     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 95.49% <0.00%> (ø)
packages/opentelemetry-metrics/src/Metric.ts 92.10% <0.00%> (ø)
packages/opentelemetry-metrics/src/export/types.ts 100.00% <ø> (ø)
packages/opentelemetry-metrics/src/types.ts 100.00% <ø> (ø)
packages/opentelemetry-plugin-http/src/http.ts 97.28% <0.00%> (ø)
...ackages/opentelemetry-shim-opentracing/src/shim.ts 87.70% <0.00%> (ø)
packages/opentelemetry-tracing/src/Span.ts 98.07% <ø> (ø)
...es/opentelemetry-core/src/trace/NoRecordingSpan.ts 31.57% <18.75%> (-68.43%) ⬇️
packages/opentelemetry-api/src/api/context.ts 84.84% <50.00%> (-2.25%) ⬇️
.../opentelemetry-exporter-collector/src/transform.ts 85.47% <66.66%> (ø)
... and 21 more

@vmarchaud vmarchaud mentioned this pull request Dec 19, 2020
30 tasks
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, don't see any blocker here

@@ -14,7 +14,7 @@
* limitations under the License.
*/

import { NoopTracerProvider } from '@opentelemetry/api';
import { NoopTracerProvider } from '@opentelemetry/api/build/src/trace/NoopTracerProvider';
Copy link
Member

Choose a reason for hiding this comment

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

does it mean that NoopTracerProvider is not available or it is in different namespace ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The NoopTracer provider is not explicitly exported by the package index so the only way to get it is by its full path. This means it is not part of the API that we claim to be "stable" and could move, change name, be removed, or similar. It also means it will not show up in autocomplete and such when people try to explore the package so it will prevent some confusion.

Base automatically changed from master to main January 25, 2021 19:26
@vmarchaud
Copy link
Member

I'm closing since the api has been moved anyway

@vmarchaud vmarchaud closed this Mar 13, 2021
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.

3 participants