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

fix: remove default version from Tracer/Meter #1828

Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jan 15, 2021

According to spec the version is optional for Tracer and `Meter.

TracerProvider/MeterProvider should not add a default to ensure that exporters only export a version if supplied during creation.

Refs:
https:/open-telemetry/opentelemetry-specification/blob/v0.7.0/specification/trace/api.md#get-a-tracer
https:/open-telemetry/opentelemetry-specification/blob/v0.7.0/specification/metrics/api.md#obtaining-a-meter

I took a look into java and python and they don't default to '*'.

According to spec the version is optional for Tracer and Meter.

TracerProvider/MeterProvider should not add a default to ensure that exporters
only export a version if supplied during creation.
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #1828 (515151d) into master (471306f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1828   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         157      157           
  Lines        5108     5108           
  Branches     1091     1091           
=======================================
  Hits         4718     4718           
  Misses        390      390           
Impacted Files Coverage Δ
packages/opentelemetry-core/src/common/types.ts 100.00% <ø> (ø)
...ackages/opentelemetry-metrics/src/MeterProvider.ts 89.47% <100.00%> (ø)
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 80.00% <100.00%> (ø)

@Oberon00
Copy link
Member

I support this change (but don't have the technical knowledge about OTel-JS to really review it).

@Flarna Flarna merged commit 1a3f3f8 into open-telemetry:master Jan 18, 2021
@Flarna Flarna deleted the optional-tracer-version branch January 18, 2021 13:02
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…en-telemetry#1838)

* test: fix TAV tests for @cucumber/cucumber and @aws-sdk/client-s3

- Switch to 'npm run --ws test-all-versions ...' for running TAV tests,
  instead of 'lerna run test-all-versions ...' because nx sets
  'npm_config_legacy_peer_deps=true' which breaks
  '@cucumber/[email protected]' install and could break other installs by
  ignoring 'peerDependencies'.
- Skip the bad '@aws-sdk/[email protected]' release in TAV tests.

Also:
- Reduce the number of versions of 'aws-sdk' and '@aws-sdk/*' packages
  test in TAV tests from 249, 143, and 132 versions currently, to 7
  each.
- Add a top-level `npm run test-all-versions` script to run that script
  in all packages that have one. This is the equivalent of the
  "test-all-versions.yml" CI workflow.

Fixes: open-telemetry#1828
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.

5 participants