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

Merge back into a single lerna monorepo #2892

Merged
merged 20 commits into from
Apr 18, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Apr 8, 2022

This is attempting to solve several pain points we've had which stem from the fact that our stable and experimental packages are not linked by lerna and aren't released together.

  • Merge into a single monorepo keeping packages in the same directories for now
  • Relax API dependencies to ^1.0.0 or >=1.0.0 <1.2.0 in order to facilitate using older APIs
  • Remove API peer dependency check
    • It checks that peer dependencies match dev dependencies, which isn't what we want

Keeping this as a draft for now

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #2892 (dc0cad6) into main (7086d5a) will decrease coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head dc0cad6 differs from pull request most recent head 9e90e2a. Consider uploading reports for the commit 9e90e2a to get more accurate results

@@            Coverage Diff             @@
##             main    #2892      +/-   ##
==========================================
- Coverage   92.76%   92.48%   -0.29%     
==========================================
  Files         163      151      -12     
  Lines        5516     4841     -675     
  Branches     1159     1022     -137     
==========================================
- Hits         5117     4477     -640     
+ Misses        399      364      -35     
Impacted Files Coverage Δ
...metry-core/src/trace/sampler/ParentBasedSampler.ts 83.87% <ø> (ø)
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100.00% <ø> (ø)
packages/opentelemetry-resources/src/Resource.ts 100.00% <ø> (ø)
...ckages/opentelemetry-core/src/common/attributes.ts 93.02% <100.00%> (-0.32%) ⬇️
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 94.49% <100.00%> (ø)
packages/opentelemetry-sdk-trace-base/src/Span.ts 99.18% <100.00%> (ø)
...ackages/opentelemetry-shim-opentracing/src/shim.ts 94.25% <100.00%> (ø)
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
... and 9 more

@legendecas
Copy link
Member

legendecas commented Apr 8, 2022

AFAICT, Lerna can only bump all changed packages to the same version -- this is not what we expect of stable packages and experimental packages release works. Is yarn workspace an available choice? I found that react is using yarn workspace and publishes packages with different versions (scheduler v0.21.0, and react v18.0.0) at one time.

@dyladan
Copy link
Member Author

dyladan commented Apr 8, 2022

AFAICT, Lerna can only bump all changed packages to the same version -- this is not what we expect of stable packages and experimental packages release works.

My current plan is to use lerna independent mode and to create automations to ensure the stable packages are versioned together and the experimental packages are versioned together.

Is yarn workspace an available choice? I found that react is using yarn workspace and publishes packages with different versions (scheduler v0.21.0, and react v18.0.0) at one time.

We considered yarn workspaces a long time ago and I can't remember exactly why it was rejected. I think it had to do with supporting old node versions but I could be wrong. NPM now also has workspace support but only for npm 7+.

@legendecas
Copy link
Member

My current plan is to use lerna independent mode and to create automations to ensure the stable packages are versioned together and the experimental packages are versioned together.

Oh, now I know lerna supports independent mode :O. This sounds great and may fit our use case.

@dyladan dyladan marked this pull request as ready for review April 13, 2022 21:23
@dyladan dyladan requested a review from a team April 13, 2022 21:23
@dyladan
Copy link
Member Author

dyladan commented Apr 14, 2022

@open-telemetry/javascript-approvers sorry for the delay but this is ready to be reviewed now. Its a lot of files changed but should be relatively easy to review.

},
"devDependencies": {
"@opentelemetry/api": "~1.1.0",
"@opentelemetry/api": "^1.0.0",
"@opentelemetry/context-async-hooks": "1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

This seems outdated

@@ -54,10 +54,10 @@
"@opentelemetry/sdk-trace-node": "~1.1.1"
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 intended to use ~ here even for components which are now in the same lerna project?

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 think we should probably pin them

@@ -42,11 +42,11 @@
"README.md"
],
"peerDependencies": {
"@opentelemetry/api": ">=1.1.0 <1.2.0",
"@opentelemetry/api": ">=1.0.0 <1.2.0",
"@opentelemetry/api-metrics": "~0.27.0"
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 intended to have api-metrics as peer dependency here but as dependency in opentelemetry-sdk-metrics-base?

packages/opentelemetry-context-async-hooks/package.json Outdated Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

I'm confused by the @opentelemetry/api version change. What benefit can we gain on that change?

@dyladan
Copy link
Member Author

dyladan commented Apr 15, 2022

I'm confused by the @opentelemetry/api version change. What benefit can we gain on that change?

The specification requires that an SDK support old API versions. This allows users to freely update their SDK without fear that dependencies which use an older API will break.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

looks fine for lerna but i'm not sure to understand why we changed back to SpanAttributes instead of Attributes ? Is this to support TS user with latest SDK but API <1.1 ?

@dyladan
Copy link
Member Author

dyladan commented Apr 15, 2022

looks fine for lerna but i'm not sure to understand why we changed back to SpanAttributes instead of Attributes ? Is this to support TS user with latest SDK but API <1.1 ?

Yes

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

SGTM

@legendecas legendecas merged commit ed2f033 into open-telemetry:main Apr 18, 2022
@dyladan dyladan deleted the merge-lerna branch April 18, 2022 15:51
@Flarna Flarna mentioned this pull request Apr 19, 2022
3 tasks
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.

4 participants