chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 #1042

blumamir · 2022-05-30T07:00:23Z

BEGIN_COMMIT_OVERRIDE
feat: update core dependencies stable ^1.3.1 experimental ^0.29.2
END_COMMIT_OVERRIDE

Which problem is this PR solving?

Update dependencies and dev dependencies to use stable ^1.3.1 and experimental ^0.29.2.
The caret is added to dev dependencies since it already pulls the latest version via transitive dependencies on other core packages. If the version is pinned, we end up with multiple core package versions which might not always be compatible. This fix should guarantee that we use the same version everywhere, which was the cause for CI failure in the past few releases.

Since this is a delicate subject, I would appreciate a thorough review and be sure that everyone interested is comfortable with this change.

Short description of the changes

Flarna · 2022-05-30T07:28:12Z

packages/opentelemetry-test-utils/package.json

-    "@opentelemetry/sdk-trace-node": "^1.0.0",
-    "@opentelemetry/resources": "^1.0.0",
-    "@opentelemetry/sdk-trace-base": "^1.0.0",
+    "@opentelemetry/sdk-trace-node": "1.2.0",


Why should we pin an old version?
Such pinning caused a lot problems in previous releases that new SDK was unusable with instrumentations from contrib.

And if pinning is the way to go - why not @opentelemetry/instrumentation?

Why should we pin an old version?

That is not an old version, but the version that the entire monorepo is aligned to currently in master.
The CI fails because we mix versions 1.2.0 and 1.3.0 since some of them are pinned in instrumentation dev dep, and others are introduced via transitive dependency on the caret range (like in the fix mentioned above).
I think we should thrive to have the same version everywhere when building and testing contrib.

Such pinning caused a lot problems in previous releases that new SDK was unusable with instrumentations from contrib.

I only changed the test-utils package which is a dev dependency, so to my understanding, it should not affect any usage for users which consume the released packages.

And if pinning is the way to go - why not @opentelemetry/instrumentation

Since instrumentations depend on it with caret, so I wanted to be align to make sure we pull the same version everywhere

blumamir · 2022-05-30T07:51:30Z

Apparently, that did not fix the issue yet.
The contrib monorepo still mixes version 1.2.0 and 1.3.0 via caret vs pinned dependencies:

all node instrumentations pin @opentelemetry/sdk-trace-base to 1.2.0 which makes sense to me.
web instrumentations depends on caret range ("@opentelemetry/sdk-trace-base": "^1.0.0", "@opentelemetry/sdk-trace-web": "^1.0.0") which pulls in 1.3.0.

I am not sure what should be the right fix here 🤷‍♂️

@Flarna do you have any suggestions?

Flarna · 2022-05-30T07:57:43Z

What about using caret versions for @opentelemetry also for dev dependencies?

Another option would be to stop trying to be compatible across versions and pin again everything and release everything once a new SDK was made. But as far as I remember the target was to avoid this.

blumamir · 2022-05-30T08:06:07Z

@Flarna - can you understand why the build is not happy:

Error: test/compatibility.test.ts(44,7): error TS2345: Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.
  Types of property 'onStart' are incompatible.
    Type '(_span: Span, _parentContext: Context) => void' is not assignable to type '(span: Span, parentContext: Context) => void'.
      Types of parameters '_span' and 'span' are incompatible.
        Type 'import("/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@opentelemetry/sdk-trace-web/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span' is not assignable to type 'import("/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span'.
          Types have separate declarations of a private property '_spanContext'.
Error: test/longTask.test.ts(67,7): error TS2345: Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.Error: test/compatibility.test.ts(44,7): error TS2345: Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.
  Types of property 'onStart' are incompatible.
    Type '(_span: Span, _parentContext: Context) => void' is not assignable to type '(span: Span, parentContext: Context) => void'.
      Types of parameters '_span' and 'span' are incompatible.
        Type 'import("/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@opentelemetry/sdk-trace-web/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span' is not assignable to type 'import("/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span'.
          Types have separate declarations of a private property '_spanContext'.
Error: test/longTask.test.ts(67,7): error TS2345: Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.

I remember it was fixed by @dyladan after releasing 1.2.0

Flarna · 2022-05-30T08:07:11Z

ok, got catched by a similar issue by myself now but not bound to @opentelemetry/contrib-test-utils.

Latest contrib and latest core packages are not compatible because caret ranges for GA packages allow minor updates but only patch for experimental.

As a result contrib instrumentations pick @opentelemetry/sdk-trace-base@1.3.0 but also @opentelemetry/instrumentation@0.28.0 which results in problems and inconsistency.

Flarna · 2022-05-30T08:12:09Z

I think we have to update all packages here to depend on ^0.29.0 and additionally use 1.3.0 or ^1.0.0 in dev-dependencies.

We should be aware that a semver minor change in experimental (0.x.y range) is similar for NPM as a semver major in GA ranges.

blumamir · 2022-05-30T08:14:38Z

I think we have to update all packages here to depend on ^0.29.0 and additionally use 1.3.0 or ^1.0.0 in dev-dependencies.

That makes sense. But still what I am missing is why ts complains about "Types have separate declarations of a private property '_spanContext'." I remember dan refactored the code to use only interfaces and not concrete types.

Flarna · 2022-05-30T08:15:37Z

I remember it was fixed by @dyladan after releasing 1.2.0

As far as I know typescript doesn't allow to mix classes from two instances of the same module (even if they have the same version). Only interfaces can match but SimpleSpanProcessor is a class.

Most likely the reason is that an instanceof check would fail as it compares the constructor instance which exists twice if package is not deduped.

Flarna · 2022-05-30T08:17:29Z

I remember dan refactored the code to use only interfaces and not concrete types.

I think only ReadableSpan was changes but can't remember in detail. It would be needed to refactor the complete SDK and remove all classes from public interface to really fix this.

blumamir · 2022-05-30T08:26:51Z

Updated the PR to use 1.3.0 and 0.29.0. Hope that will not cause any new issues

blumamir · 2022-05-30T08:37:16Z

Now @opentelemetry/host-metrics fails to build with many errors such as:

src/metric.ts:173:7 - error TS2554: Expected 1-2 arguments, but got 3.

I guess there were some changes in the metrics apis which need to be applied, but I am not very familiar with the metrics 😕

codecov · 2022-05-30T12:35:16Z

Codecov Report

Merging #1042 (84a97a9) into main (3e2f9c5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1042   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files          13       13           
  Lines         856      856           
  Branches      178      178           
=======================================
  Hits          821      821           
  Misses         35       35

blumamir · 2022-05-30T12:59:47Z

I updated all versions and fixed the built.

node tests are green, but browser tests fails because:

ERROR in ./node_modules/@opentelemetry/resources/build/esm/detectors/HostDetector.js 54:0-36
Module not found: Error: Can't resolve 'os' in '/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/web/opentelemetry-instrumentation-document-load/node_modules/@opentelemetry/resources/build/esm/detectors'

I have little experience with browser workflows. Any ideas on how to resolve this?

rauno56 · 2022-06-02T13:52:02Z

With the release of open-telemetry/opentelemetry-js#3013 this should now be fine?

Flarna · 2022-06-02T14:25:04Z

With the release of open-telemetry/opentelemetry-js#3013 this should now be fine?

Don't think so because the actual fix in open-telemetry/opentelemetry-js#3004 changed @opentelemetry/resource but the stable components were not released yet.

…ions

…emetry-js-contrib into test-utils-pin-versions

blumamir · 2022-06-08T10:07:53Z

CI is finally green ✅

packages/opentelemetry-browser-extension-autoinjection/package.json

chore(test-utils): pin dev dependencies to across the repo

1ee76a1

blumamir requested a review from a team May 30, 2022 07:00

Flarna reviewed May 30, 2022

View reviewed changes

chore: upgrade all dependencies to core 1.3.0/0.29.0

a2d7bfa

github-actions bot assigned blumamir, legendecas, naseemkullah, NathanielRN, obecny, rauno56 and willarmiros May 30, 2022

blumamir mentioned this pull request May 30, 2022

fix(resources): extend ResourceAttributes interface to comply with spec open-telemetry/opentelemetry-js#2924

Merged

7 tasks

Flarna mentioned this pull request May 30, 2022

getNodeAutoInstrumentations type error when initializing tracing in TypeScript open-telemetry/opentelemetry-js#2964

Closed

Amir Blum added 2 commits May 30, 2022 14:36

fix(host-metrics): fix build with new api

917ee5b

fix(host-metrics): fix tests

b6d205c

blumamir changed the title ~~chore(test-utils): pin dev dependencies to across the repo~~ chore(test-utils): update core dependencies stable 1.3.0 experimental 0.29.0 May 30, 2022

blumamir mentioned this pull request May 30, 2022

fix(resources): fix browser compatibility for host and os detectors open-telemetry/opentelemetry-js#3004

Merged

6 tasks

Merge branch 'main' into test-utils-pin-versions

373e8a0

Amir Blum added 3 commits June 8, 2022 11:13

Merge remote-tracking branch 'upstream/main' into test-utils-pin-vers…

386115d

…ions

chore: use core caret ^0.29.2 and ^1.3.1

96408a7

Merge branch 'test-utils-pin-versions' of github.com:blumamir/opentel…

bfdb4cf

…emetry-js-contrib into test-utils-pin-versions

blumamir changed the title ~~chore(test-utils): update core dependencies stable 1.3.0 experimental 0.29.0~~ chore(test-utils): update core dependencies stable ^1.3.1 experimental ^0.29.2 Jun 8, 2022

Flarna approved these changes Jun 8, 2022

View reviewed changes

legendecas approved these changes Jun 8, 2022

View reviewed changes

blumamir changed the title ~~chore(test-utils): update core dependencies stable ^1.3.1 experimental ^0.29.2~~ chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 Jun 8, 2022

Flarna mentioned this pull request Jun 8, 2022

dependency version mismatch in instrumentation-express #1051

Closed

dyladan reviewed Jun 8, 2022

View reviewed changes

packages/opentelemetry-browser-extension-autoinjection/package.json Outdated Show resolved Hide resolved

fix: forgotten version

84a97a9

dyladan approved these changes Jun 8, 2022

View reviewed changes

dyladan linked an issue Jun 8, 2022 that may be closed by this pull request

dependency version mismatch in instrumentation-express #1051

Closed

dyladan removed a link to an issue Jun 8, 2022

dependency version mismatch in instrumentation-express #1051

Closed

dyladan merged commit 141b155 into open-telemetry:main Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 #1042

chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 #1042

blumamir commented May 30, 2022 •

edited by dyladan

Loading

Flarna May 30, 2022

blumamir May 30, 2022

blumamir commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

Flarna commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

Flarna commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

blumamir commented May 30, 2022

codecov bot commented May 30, 2022 •

edited

Loading

blumamir commented May 30, 2022

rauno56 commented Jun 2, 2022

Flarna commented Jun 2, 2022

blumamir commented Jun 8, 2022

chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 #1042

chore: update core dependencies stable ^1.3.1 experimental ^0.29.2 #1042

Conversation

blumamir commented May 30, 2022 • edited by dyladan Loading

Which problem is this PR solving?

Short description of the changes

Flarna May 30, 2022

Choose a reason for hiding this comment

blumamir May 30, 2022

Choose a reason for hiding this comment

blumamir commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

Flarna commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

Flarna commented May 30, 2022

Flarna commented May 30, 2022

blumamir commented May 30, 2022

blumamir commented May 30, 2022

codecov bot commented May 30, 2022 • edited Loading

Codecov Report

blumamir commented May 30, 2022

rauno56 commented Jun 2, 2022

Flarna commented Jun 2, 2022

blumamir commented Jun 8, 2022

blumamir commented May 30, 2022 •

edited by dyladan

Loading

codecov bot commented May 30, 2022 •

edited

Loading