feat!: use semver to determine API compatibility #1772

dyladan · 2020-12-21T22:22:00Z

This removes the manual step of the API compatibility version and replaces it with a proper semver check. The expected range is ^${VERSION} where VERSION is the version of the API instance attempting to acquire the global API.

/cc @Flarna since this was your idea I would appreciate your 👀

THIS IS INCOMPATIBLE WITH PAST API VERSIONS AND IS A HARD BREAKING CHANGE

packages/opentelemetry-api/src/api/global-utils.ts

codecov · 2020-12-22T16:06:35Z

Codecov Report

Merging #1772 (8310269) into master (b4c9e40) will decrease coverage by 0.13%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master    #1772      +/-   ##
==========================================
- Coverage   92.18%   92.05%   -0.14%     
==========================================
  Files         167      164       -3     
  Lines        5845     5560     -285     
  Branches     1256     1208      -48     
==========================================
- Hits         5388     5118     -270     
+ Misses        457      442      -15

Impacted Files	Coverage Δ
packages/opentelemetry-api/src/api/metrics.ts	`78.94% <50.00%> (+15.31%)`	⬆️
...ges/opentelemetry-api/src/internal/global-utils.ts	`92.00% <92.00%> (ø)`
packages/opentelemetry-api/src/api/context.ts	`88.88% <100.00%> (+1.79%)`	⬆️
packages/opentelemetry-api/src/api/propagation.ts	`100.00% <100.00%> (+7.14%)`	⬆️
packages/opentelemetry-api/src/api/trace.ts	`95.83% <100.00%> (+2.97%)`	⬆️
packages/opentelemetry-api/src/internal/semver.ts	`100.00% <100.00%> (ø)`
...emetry-core/src/platform/node/RandomIdGenerator.ts	`87.50% <0.00%> (-6.25%)`	⬇️
...emetry-instrumentation-xml-http-request/src/xhr.ts
...mentation-xml-http-request/src/enums/EventNames.ts
packages/opentelemetry-plugin-fetch/src/fetch.ts
... and 3 more

packages/opentelemetry-api/src/api/metrics.ts

packages/opentelemetry-api/src/api/context.ts

packages/opentelemetry-api/src/api/propagation.ts

packages/opentelemetry-api/src/api/trace.ts

packages/opentelemetry-api/package.json

obecny

generally looks fine, I have just concerns with regards the error handling which should be imho changed, so we don't break / stop the end user functionality

obecny · 2021-01-07T16:16:24Z

packages/opentelemetry-api/src/internal/global-utils.ts

+  const api = _global[GLOBAL_OPENTELEMETRY_API_KEY]!;
+  if (api[type]) {
+    // already registered an API of this type
+    throw new Error(


shouldn't we warn here only instead of throwing an error ?

Yeah does the spec require us to never throw on the API ?

The spec strongly discourages throwing. I can change it, but the behavior of doing nothing is potentially confusing. This should only be called during trace setup and never again so it seems unlikely to throw. I thought it would be fine in this case. I can change it to warn though if that's better.

I would prefer a warn since users generally think there are errors anyway (since the console logger doesn't log the level by default)

there is no logger here because we're in the API. It has to be console.warn. Is that OK?

any CLI tool written in nodejs could potentially have an expected output to be parsed by other tools. If we log unexpectedly we change that output.

Simple example, a cli that scrapes the github API and outputs the open issues on a project as JSON to be consumed by jq and other tools.

Exactly. It's not nodejs which runs into issues it's the specific user application.
I remember at least one case where I broke an application by logging to stderr because this was seen as error by the caller.
And logging to sdtout tends to break everything where a caller reads result from stdout.

If documentation clearly states that an API may throw it's up to the user (and all our samples) to add a try/catch and log/exit/...
A log to console can't be catched by user (except by monkey patching console but I don't think we want users to do this).

We could change the API to return a boolean instead of throwing. But this results in loosing the reason why it failed. We could return an error string or similar but I don't think these are nice APIs.

This is an API only used once during startup. Throwing there doesn't force users to clutter their whole codebase with try/catches. Therefore I think it's ok to throw here.

If documentation clearly states that an API may throw it's up to the user (and all our samples) to add a try/catch and log/exit/...

I don't think expecting users to try/catch when trying to setup the sdk or a instrumentation would be a good solution either.

I see it as analogous to something like http server setup. If you try to bind to a port which is already bound, it throws. Because duplicate registration is startup/setup time it is most likely never throws or throws every time, not flaky or sometimes happening.

Guys we can't break user application just because the otel configuration is wrong or because otel is missing something. Imagine the simple situation when you release a new version on production you then restart all the nodes and suddenly all production is down because the otel has wrong configuration. We can't do this. If otel fails all what should happen is to show user the error but never stop or break the application. The application should work fine in any situation.

obecny · 2021-01-07T16:16:35Z

packages/opentelemetry-api/src/internal/global-utils.ts

+
+  if (api.version != VERSION) {
+    // All registered APIs must be of the same version exactly
+    throw new Error(


obecny · 2021-01-07T16:20:12Z

packages/opentelemetry-api/src/internal/semver.ts

+
+  const myVersionMatch = ownVersion.match(re);
+  if (!myVersionMatch) {
+    throw new Error('Cannot parse own version');


I think that the whole function should work as typical boolean check and in this case it should return false and maybe just log warning. Otherwise it might stop / break the user system

If it can't parse it's own version that is something that will fail every single time and nothing the user can do about it. In this case I think throwing is the appropriate response since it's a bug that must be fixed. The throw should make this trip in tests and never allow us to release with a version which can't be parsed.

Flarna · 2021-01-08T17:24:28Z

packages/opentelemetry-api/src/internal/semver.ts

+      return false;
+    }
+
+    acceptedVersions.add(version);


Is it intended to ignore the pre-release part in comparison?
The semver module doesn't do this on default, only if includePrerelease is specified.

Yes the prerelease part is ignored. A prereleased version should be treated by the comparison as if it was the released version. I may want to release 2.3.1-rc to ensure it works as expected before releasing 2.3.1 and I would not want the check to behave differently for the released v prereleased version.

dyladan · 2021-02-19T16:11:49Z

open-telemetry/opentelemetry-js-api#6

…pen-telemetry#1772) * feat(auto-instrumentations-node): Expose getting resource detectors The auto instrumentation package provides users with a very easy way to instrument their application, either from the commandline or programatically. Users wishing the configure these instrumentations can also call `getNodeAutoInstrumentations`. This commit also exposes `getResourceDetectorsFromEnv`, so that users configuring or somehow wishing to change the easy to use autoinstrumentation entrypoint can also use the preconfigured resource detectors. Since the autoinstrumentation package sets its exports, this function was previously inaccessible. Usage can look like this: ```js const { getNodeAutoInstrumentations, getResourceDetectorsFromEnv } = require('@opentelemetry/auto-instrumentations-node'); const opentelemetry = require('@opentelemetry/sdk-node'); const sdk = new opentelemetry.NodeSDK({ instrumentations: getNodeAutoInstrumentations({ '@opentelemetry/instrumentation-fs': { enabled: false }, }), resourceDetectors: getResourceDetectorsFromEnv(), }); // use the sdk ``` Workarounds can be found [elsewhere in the opentelemetry repos](https://github.com/open-telemetry/opentelemetry-demo/blob/855bde3588b0fe85e500bec185dc2f311b15f98a/src/paymentservice/opentelemetry.js#L28-L38). These can be fixed once a release is made. Resolves open-telemetry#1531. * style: Align indentation --------- Co-authored-by: Amir Blum <amirgiraffe@gmail.com>

open-telemetry#1792) Better align with the other exports. Follow-up to open-telemetry#1772.

dyladan added 2 commits December 21, 2020 17:15

feat!: use semver to determine API compatibility

e185eac

chore: use appropriate semver range

3198c00

dyladan added API labels Dec 21, 2020

dyladan requested review from legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners December 21, 2020 22:22

chore: fix build

706dd64

Flarna reviewed Dec 21, 2020

View reviewed changes

packages/opentelemetry-api/src/api/global-utils.ts Outdated Show resolved Hide resolved

Flarna reviewed Dec 21, 2020

View reviewed changes

packages/opentelemetry-api/src/api/global-utils.ts Outdated Show resolved Hide resolved

Flarna reviewed Dec 21, 2020

View reviewed changes

packages/opentelemetry-api/src/api/global-utils.ts Outdated Show resolved Hide resolved

dyladan added 5 commits December 22, 2020 09:20

chore: enumerate api types

154584d

chore: all api registrations must be the same version

abfd4a9

chore: deduplicate version checking code

abfbdaa

chore: fix api registration in tests

c936feb

chore: fix API registration in validation-server

426a3d6

dyladan added 3 commits December 22, 2020 11:16

chore: instrumentation test api registration

4769803

Merge remote-tracking branch 'origin/master' into semver-global

948b89a

chore: fix noop logger import

42a5824

obecny reviewed Dec 29, 2020

View reviewed changes

dyladan added 4 commits January 6, 2021 09:19

Merge remote-tracking branch 'origin/master' into semver-global

39541ee

chore: use const for api names

8d82389

chore: remove semver dependency

bb718d9

chore: add tests for api semver check

efcf981

chore: add jsdocs to semver functions

8310269

obecny reviewed Jan 7, 2021

View reviewed changes

Flarna reviewed Jan 8, 2021

View reviewed changes

Base automatically changed from master to main January 25, 2021 19:26

dyladan requested a review from johnbley as a code owner January 25, 2021 19:26

dyladan mentioned this pull request Feb 8, 2021

feat(diag-logger): introduce a new global level api.diag for internal diagnostic logging #1880

Merged

dyladan mentioned this pull request Feb 18, 2021

Use semver to determine API compatibility open-telemetry/opentelemetry-js-api#5

Closed

dyladan closed this Feb 19, 2021

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

refactor(auto-instrumentations-node): Name export getResourceDetectors (

24ffb3b

open-telemetry#1792) Better align with the other exports. Follow-up to open-telemetry#1772.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: use semver to determine API compatibility #1772

feat!: use semver to determine API compatibility #1772

dyladan commented Dec 21, 2020

codecov bot commented Dec 22, 2020 •

edited

Loading

obecny left a comment

obecny Jan 7, 2021

vmarchaud Jan 7, 2021

dyladan Jan 7, 2021

vmarchaud Jan 7, 2021

dyladan Jan 7, 2021

dyladan Jan 8, 2021

Flarna Jan 8, 2021

vmarchaud Jan 8, 2021

dyladan Jan 8, 2021

obecny Jan 8, 2021

obecny Jan 7, 2021

obecny Jan 7, 2021

dyladan Jan 7, 2021

Flarna Jan 8, 2021

dyladan Jan 8, 2021

dyladan commented Feb 19, 2021

feat!: use semver to determine API compatibility #1772

feat!: use semver to determine API compatibility #1772

Conversation

dyladan commented Dec 21, 2020

codecov bot commented Dec 22, 2020 • edited Loading

Codecov Report

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Feb 19, 2021

codecov bot commented Dec 22, 2020 •

edited

Loading