new v0.18.1 released as patched but is actually a breaking change version

[blumamir]

Yesterday, a new patch version v0.18.1 was published.
This version depended on a new major version of @opentelemetry/api: ^1.0.0-rc.0.

Installing multiple versions of api package in the same application is not compatible as the symbols to access global apis such as context and propagation is different, causing different otel components to reference different globals.

Example of the issue that causes infinity loop on new v0.18.1:

// this code is referencing "@opentelemetry/api": "^0.18.0", thus setting the context on api major version 0.
        context.with(suppressInstrumentation(context.active()), () => {
            this.exporter.export([span], () => {});
        });

The exporter (v0.18.0) is using http module to export, which is calling http instrumentation (v0.18.0), which has dependency on "@opentelemetry/tracing": "^0.18.1". So when @opentelemetry/instrumentation-http is calling tracer startSpan, the invoked function is coming from @opentelemetry/[email protected] which has a dependency on @opentelemetry/[email protected], thus suppressInstrumentation has no effect which means exported spans are instrumented in an infinity loop.

Since the release of v0.18.1, any new installation of our (Aspecto), previously working SDK which depends on exact versions v0.18.0 is currently crashing the client application due to this issue.

This new version, which is marked as patch, will also find its way into all the plugins in contrib repo and in opentelemetry-ext-js which depends on api version ^0.18.0 but will receive 1.0.0-rc.0 version via dependency on ^0.18.0 of core packages.

I think the new core packages versions should be 0.19.0, and v0.18.1 should be unpublished from npm.

[dyladan]

https://github.com/open-telemetry/opentelemetry-js/releases/tag/v0.18.2

[Flarna]

Installing multiple versions of api package in the same application is not compatible as the symbols to access global apis such as context and propagation is different, causing different otel components to reference different globals.

Is this something we can/should improve in API? The main idea was to return Noop instances if version doesn't match but it should not crash nor end up in endless loops.

[blumamir]

Is this something we can/should improve in API? The main idea was to return Noop instances if version doesn't match but it should not crash nor end up in endless loops.

I believe that it does return NOOP. Our endless loop happened due to the context not being set and read by the same api version, causing suppressInstrumentation not to work - which is a recipe for infinite loops.
There might be other issues such as propagation not working etc, but it will probably resolve when we move to api v.1.0.0 everywhere.

[vmarchaud]

Is this something we can/should improve in API? The main idea was to return Noop instances if version doesn't match but it should not crash nor end up in endless loops.

We were discussing this in the maintainers conv and @dyladan suggested that the main problem is that components should have the API as peer dependency so they don't use their own api version (on major bump, npm/yarn will complain that the peer dependency isn't there if the user bump api without also bumping component), this would theorically avoid this problem.

[dyladan]

I had not thought of the suppressInstrumentation case. I wonder if the NOOP context manager should return a context with instrumentation suppressed instead of returning the root context?

[blumamir]

@dyladan to me it sounds like a good idea which will increase the robustness of the project - failing more securely in case of issues.
If there is no context manager installed, I see no point in returning a full "recording span" from startSpan.

[dyladan]

Well there is no requirement to use a context manager currently. It is perfectly reasonable to manually pass around context objects without a context manager if you have tight performance concerns or unique use-cases.

In this case you wouldn't expect to use the context from the context manager at all so returning a suppressed context is OK.

[blumamir]

If the user is managing the context himself, then he will not call context.active() (directly, or via default parameter to context parameter in startSpan), so it should not be a problem I believe.

In case user meant to handle context manually, but for some reason context.active() is called, then returning suppressed instrumentation context will protect him from potential issues.