Pin our own dependencies

[dyladan]

Currently it is not possible for users to pin versions without a package-lock because we depend on "carat" versions of our own packages. If we release a new patch version, end users will get the new versions even if they are trying to pin dependency versions which doesn't give them a chance to test new versions.

/cc @blumamir

[Flarna]

Do you refer to dependencies between packages within one repo (e.g. @opentelemetry/node depends on @opentelemetry/core) or also cross repos (contrib => core => api)?

For packages within on repo it sounds reasonable as they are anyway released in one shot.

Usually it's a good thing to get patch updates like security fixes,.. automatically.

[dyladan]

Within repo. API should still be at least ~ if not ^

[Flarna]

isn't there still a risk to get this mixed up if people use something from contrib where core components are referenced by ~ or ^?

[blumamir]

isn't there still a risk to get this mixed up if people use something from contrib where core components are referenced by ~ or ^?

You are right, unfortunately
It will not eliminate the problem altogether, but at least it will guarantee that core packages are same-version compatible between themselves (not possible that installed @opentelemetry/exporter-collector:0.18.0 is using @opentelemetry/tracing:0.18.1). It will also decrease the problem surface area I believe, introducing fewer uncontrollable version changes to existing projects on new releases.

Most users I suppose are installing dependencies with caret @opentelemetry/tracing:^0.18.0 as it's the default for npm/yarn, and for them the proposed change will not make any difference as they will already get the patch/minor release on any install or upgrade via the main dependency.

If you have an idea how to maintain pinning dependencies from contib packages as well, that would make me super happy :)

[dyladan]

The only way to make contrib also pinnable would be to remove ranges altogether and release a new contrib each time core is released.

[Flarna]

I think in a real app it should be not that bad.
Consider you reference @opentelemertry/api, @opentelemetry/node and @opentelemetry/instrumentation-redis in your package.json.
@opentelemetry/node can pin all deps in core even if @opentelemetry/instrumentation-redis allows a newer variant.

As far as I know npm installs only a second instance if the version ranges don't allow de duplication.
So if @opentelemetry/node is "0.18.0" and @opentelemetry/instrumentation-redis requests "^0.18.0" npm would install "0.18.0" even if 0.18.1 is available.

[dyladan]

As far as I know npm installs only a second instance if the version ranges don't allow de duplication.

Yes that is correct

[blumamir]

Thanks for the help.
I tried to test it and for some reason it doesn't work on my setup:

I created a demo app with these dependencies:

    "@opentelemetry/api": "0.18.0",
    "@opentelemetry/core": "0.18.0",
    "@opentelemetry/plugin-redis": "0.14.0",

and in my node_modules root directory there is installation of api and core v0.18.0 as expected (the exact version from package.json), but under node_modules/@opentelemetry/plugin-redis/node_modules there is a second installation of api and core with versions @opentelemtry/api@0.18.1 (current ^0.18.0 for api), and @opentelemetry/core@0.18.2 (current ^0.18.0 for core).

I am working with yarn, not npm, maybe that is the issue?

Anyway, I think this case is less important then the amount of time we put into it. I was just trying to make my installations more stable and predictable, by getting as little code as possible "automatically installed" via semver ranges.
As long as new patch versions are not introducing any breaking or compatibility changes with other minor versions - it shouldn't really matter.

[blumamir]

I have another example where current version dependency breaks.

Consider this code:

import { NodeTracerProvider } from '@opentelemetry/node';
import { SimpleSpanProcessor, ConsoleSpanExporter } from '@opentelemetry/tracing';

const provider = new NodeTracerProvider();
// SimpleSpanProcessor is imported from 0.18.0
// addSpanProcessor is imported from @opentelemetry/node@0.18.0, which expect SpanProcessor with the interface from 0.18.2 due to it's caret dependency
provider.addSpanProcessor(new SimpleSpanProcessor(new ConsoleSpanExporter()));

{
  "name": "otel-versions",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "scripts": {
    "start": "ts-node-dev app.ts"
  },
  "devDependencies": {
    "ts-node-dev": "^1.1.6",
    "typescript": "^4.2.3"
  },
  "dependencies": {
    "@opentelemetry/node": "0.18.0",
    "@opentelemetry/tracing": "0.18.0"
  }
}

To my understanding, this is a perfectly valid setup of opentelemetry, but when compiled, it generates the following error:

app.ts:5:27 - error TS2345: Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.
  Types of property 'onStart' are incompatible.
    Type '(_span: Span) => void' is not assignable to type '(span: Span, parentContext: Context) => void'.
      Types of parameters '_span' and 'span' are incompatible.
        Type 'import("/Users/amirblum/repos/otel-versions/node_modules/@opentelemetry/node/node_modules/@opentelemetry/tracing/build/src/Span").Span' is not assignable to type 'import("/Users/amirblum/repos/otel-versions/node_modules/@opentelemetry/tracing/build/src/Span").Span'.
          Types have separate declarations of a private property '_ended'.

provider.addSpanProcessor(new SimpleSpanProcessor(new ConsoleSpanExporter()));

This happens because @opentelemetry/node@0.18.0 is dependent on "@opentelemetry/tracing": "^0.18.0", which means that the code above is mixing @opentelemetry/tracing@0.18.0 and @opentelemetry/tracing@0.18.2. Even though 0.18.2 is a patch release, the interface for SpanProcessor changed which is a typescript breaking change and fails compilation.

To my understanding, for the project to currently work, it requires that end users install core packages with caret (^0.18.0).
I think that if this is the statement, then we should document it in a clear way.

I would prefer of course to support installation of pinned versions (0.18.0), which increases stability as new patched versions are not forced pushed to users who choose to use a tested-pinned version.

[dyladan]

I am working with yarn, not npm, maybe that is the issue?

Can you try with NPM?

the interface for SpanProcessor changed which is a typescript breaking change and fails compilation.

The interface actually didn't change, but a new private property was apparently added. tracing.TracerProvider.addSpanProcessor takes a tracing.SpanProcessor when it should probably take an api.SpanProcessor as an argument.

[dyladan]

I would prefer of course to support installation of pinned versions (0.18.0), which increases stability as new patched versions are not forced pushed to users who choose to use a tested-pinned version.

So would I. I think the only way to do this is for us to pin our own dependencies. I looked at angular and it appears they do the same thing in order to support pinning.

[blumamir]

Can you try with NPM?

Tried with NPM and it works. So this approach will work well for npm users but not for yarn users

[dyladan]

Wonder if yarn has any flags to force the npm behavior. Could this be considered a yarn bug?

[blumamir]

The interface actually didn't change, but a new private property was apparently added.

Cool, thanks for looking into it. My point is that patch versions should not break application/compilation, but it's really easy for breaking behavior to sneak in by mistake.

tracing.TracerProvider.addSpanProcessor takes a tracing.SpanProcessor when it should probably take an api.SpanProcessor as an argument.

Should we open an issue for that?

[blumamir]

So would I. I think the only way to do this is for us to pin our own dependencies. I looked at angular and it appears they do the same thing in order to support pinning.

I vote for pinning them. Actually, I can't think of a drawback to pining own dependencies. Users which install core packages with caret, already have the latest version of the direct dependency, and it will install the latest version of pinned sub dependency as well

[blumamir]

Wonder if yarn has any flags to force the npm behavior. Could this be considered a yarn bug?

Tested it again with yarn, and I see the same behavior as npm. Not sure how to reproduce the original setup in which I observed the problem, but maybe it was just an issue on my machine.

[Flarna]

The interface actually didn't change, but a new private property was apparently added. tracing.TracerProvider.addSpanProcessor takes a tracing.SpanProcessor when it should probably take an api.SpanProcessor as an argument.

Seems it's by far easier to introduce a breaking change via typescipt compared to javascript. Is anyone aware of a guideline what needs to be done to avoid such issues?

[dyladan]

This is only "breaking" so far as if you have multiple versions of @opentelemetry/tracing in your node_modules directory due to some dependency version mismatch, you can't use the SpanProcessor from one as the argument to TracerProvider.addSpanProcessor in the other.

In general, the way to avoid this is to use interfaces everywhere possible instead of using concrete classes as the type for function arguments and returns.

[dyladan]

In this particular case, the SpanProcessor is already an interface, but it refers to the concrete Span class from the tracing module instead of the API Span interface.

[dyladan]

In this particular case, the SpanProcessor is already an interface, but it refers to the concrete Span class from the tracing module instead of the API Span interface.

Created #2075 for this