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.

Sign up for GitHub

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

Jump to bottom

feat: add support for API 1.1.x #2737

Merged
merged 11 commits into from
Jan 31, 2022
Merged

feat: add support for API 1.1.x #2737

merged 11 commits into from
Jan 31, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jan 24, 2022

Prepare a draft PR to get schemaUrl feature as quickly as possible after API release.

Depends on open-telemetry/opentelemetry-js-api#154

@dyladan dyladan requested a review from a team January 24, 2022 21:58
@codecov
Copy link

codecov bot commented Jan 24, 2022 •
edited
Loading

Codecov Report

Merging #2737 (393c677) into main (ce5f7f7) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 393c677 differs from pull request most recent head d20010d. Consider uploading reports for the commit d20010d to get more accurate results

@@           Coverage Diff           @@
##             main    #2737   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         159      159           
  Lines        5444     5444           
  Branches     1141     1142    +1     
=======================================
  Hits         5078     5078           
  Misses        366      366
Impacted Files Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 94.49% <100.00%> (ø)

@dyladan dyladan marked this pull request as draft January 24, 2022 22:03
@dyladan
Copy link
Member Author

dyladan commented Jan 24, 2022

Converting to draft until API release

@dyladan dyladan marked this pull request as ready for review January 27, 2022 16:57
@dyladan dyladan changed the title feat: add tracer options to getTracer feat: add support for API 1.1.x Jan 27, 2022
@dyladan
Copy link
Member Author

dyladan commented Jan 27, 2022

@open-telemetry/javascript-maintainers please review this ASAP as the API is already released and I want to release the SDK in a timely manner

@dyladan dyladan merged commit ee2342b into open-telemetry:main Jan 31, 2022
@dyladan dyladan deleted the prepare-for-api branch January 31, 2022 18:08
@dyladan dyladan added the enhancement New feature or request label Jan 31, 2022
Flarna
Flarna reviewed Feb 3, 2022
View reviewed changes
packages/opentelemetry-sdk-trace-node/package.json
@@ -59,7 +59,7 @@
"typescript": "4.4.4"
},
"peerDependencies": {
"@opentelemetry/api": ">=1.0.0 <1.1.0"
"@opentelemetry/api": ">=1.0.0 <1.2.0"
Copy link
Member

Choose a reason for hiding this comment

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

Thinking one more about this now with #2755 (comment) in mind it seems to be not that good to allow more then one minor in SDK.

If SDK ends up in using 1.0.x because the de-duplication of package manager decide so but other components use 1.1.x these other components will have issues as the version check in API signals incompatibility.

I guess the SDK must use the highest minor.

@Flarna Flarna mentioned this pull request Feb 7, 2022
Open
2 tasks
@Flarna
Copy link
Member

Flarna commented Feb 7, 2022

This change caused CI to fail for nodejs 16 and the version range in SDKs was narrowed in #2759 to allow 1.1 only.
followup: #2769

@Flarna Flarna mentioned this pull request Feb 15, 2022
Merged
@dyladan dyladan mentioned this pull request Mar 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flarna Flarna Flarna approved these changes

@MSNev MSNev MSNev approved these changes

@blumamir blumamir blumamir approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dyladan @Flarna @vmarchaud @blumamir @MSNev