docs: align all supported versions to a common format #2196

blumamir · 2024-05-11T04:55:22Z

This PR is not breaking any existing behavior, it only aligns everywhere to use a consistent versioning syntax and documentation, as well as capping supportedVersions for any existing instrumentation

make sure there is upper limit on all supported versions so that they do not attempt to patch a new major version
align the range to a common format. It seems many instrumentation where using >=x.y.z <w writing so I used it
README.md: added where missing, moved to always be after Installation section, added package name and npm link before version range to give more context.
added section to GUIDELINES.md
replace all the use of astrix * for unordered list style in makrdown to use dash -, it causes errors in markdown linter after introducing new sections.

related: open-telemetry/opentelemetry-js#4693
related: open-telemetry/opentelemetry-js#4696

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

…ed-versions

linux-foundation-easycla · 2024-05-18T06:21:58Z

The committers listed above are authorized under a signed CLA.

✅ login: blumamir / name: Amir Blum (6a367bf, 9d76835, 8582727, 7b0e95d, 4e31da4, af9d138, 5e66cff, c94ea20, 1602d53, f2cb90d, fa20292, 548b8c4, 7186912, 622a97c, f471dff, 0c07a5c, 79f4fe8, 8405555, b84dbf3, 9d269ae, 0c7a463, 47713dd, eecf73f, 0d238c3, 47cdc01, 5b5375a, 7f71c9a, 8455d96, 857fea3, 1ac55f1, c188978, d5a4290, 87f84dd, 096f5c5, 831c32d, e5ca81b, eba1e0d, b354455, b2bafbe, 88ab4fa, bb8c2bb, cf866e1, 4168fea, 35ab033, 93e2961, cb72c24, 3dff4b5, 7eacab4, aaca52d, 314f302, fc98fcd, ea5718a, e02a7c3, 1d02804)

blumamir · 2024-05-18T07:12:46Z

@trentm @JamieDanielson I think I addressed all the comments from your reviews. Since these are a lot of changes, please have a look again. I think EasyCLA is having hard time with the commits done via the code suggestion feature of PR comments in the UI. I will squash and merge all the commits after your review so to not lose and existing comments.

The one last thing I am not sure about is if we should choose caret ^ range when the version range is just one major version. I am split on this, but tend to prefer replacing it with >=1.0.0 <2 instead of ^1.0.0. WDYT?

Thanks again for those great comments suggestions and catches.

trentm

Some nits. I think it would be good to not have this labelled as purely a "docs" PR because it does include code changes for a few modules to add an upper limit on the instrumented versions.

trentm · 2024-05-21T23:23:33Z

plugins/node/instrumentation-amqplib/src/amqplib.ts

@@ -74,6 +74,8 @@ import {
 } from './utils';
 import { VERSION } from './version';

+const supportedVersions = ['>=0.5.5 <1'];


nit: Perhaps this is academic because there isn't an amqplib 1.x release, and I'm guessing there isn't one in the short term. However, this is a "docs:" PR that changes instr-amqplib code. Perhaps mention this change is adding an upper limit for this instr?

I allowed myself to do that, because I was looking at the current major and capped it there. While very unlikely that any user will be affected by this, I don't mind documenting it just in case. Do you think the scope should change from docs to fix!?

Can the scope be docs, chore: ...? I suppose it could be fix as well. I don't think it is realistically a breaking change for any of the instrumentations changed, but I'm not opposed to a very conservative fix!.

FWIW I prefer to err on the side of conservative, and point out possible behavior changes even if they are unlikely. Renaming from docs tochore could be sufficient here (...depending on the definition of chore, which differs across resources as it could be "non-source changes" - disqualifying this - or "routine tasks" - where this fits... but that might be too far off-course to have that convo here 😅 )

trentm · 2024-05-21T23:27:50Z

plugins/node/opentelemetry-instrumentation-generic-pool/src/instrumentation.ts

@@ -40,7 +40,7 @@ export default class Instrumentation extends InstrumentationBase {
    return [
      new InstrumentationNodeModuleDefinition(
        MODULE_NAME,
-        ['>=3'],
+        ['^3.0.0'],


nit: Similar to the amqplib point above. This PR is slightly more than just a docs PR.

trentm · 2024-05-21T23:28:07Z

plugins/node/opentelemetry-instrumentation-graphql/README.md

@@ -21,7 +21,7 @@ npm install @opentelemetry/instrumentation-graphql

 ### Supported Versions

-`^14.0 | ^15.0 | ^16.0`
+- [`graphql`](https://www.npmjs.com/package/graphql) versions `'>=14.0.0 <17'`


Suggested change

- [`graphql`](https://www.npmjs.com/package/graphql) versions `'>=14.0.0 <17'`

- [`graphql`](https://www.npmjs.com/package/graphql) versions `>=14.0.0 <17`

trentm · 2024-05-21T23:40:39Z

GUIDELINES.md

+
+Specific guidelines for different cases:
+
+- For `Different Modules`, instrumentations can use an upper limit on patched packages but it is unknown which future versions of the instrumented package will continue to use it. Thus it is ok to use an open upper limit `>=1.2.3` for the instrumented package.


Suggested change

- For `Different Modules`, instrumentations can use an upper limit on patched packages but it is unknown which future versions of the instrumented package will continue to use it. Thus it is ok to use an open upper limit `>=1.2.3` for the instrumented package.

- For `Different Modules`, instrumentations can use an upper limit on patched packages but it is unknown which future versions of the instrumented package will continue to use it. Thus it is ok to use an open upper limit, for example `>=1.2.3`, for the instrumented package.

trentm · 2024-05-21T23:42:00Z

GUIDELINES.md

+- [ ] Test the instrumentation with the new version to ensure it works as expected.
+- [ ] Update the supported versions list in the instrumentation code, perhaps with different patches and additional `InstrumentationNodeModuleDefinition`s that target the new version.
+- [ ] Update the README file to reflect the support for new versions.
+- [ ] For instrumentations that use test-all-versions `.tav.yaml`, add the new version to the list of versions to test.


Suggested change

- [ ] For instrumentations that use test-all-versions `.tav.yaml`, add the new version to the list of versions to test.

- [ ] For instrumentations that use test-all-versions `.tav.yml`, add the new version to the list of versions to test.

JamieDanielson · 2024-05-22T21:44:39Z

The one last thing I am not sure about is if we should choose caret ^ range when the version range is just one major version. I am split on this, but tend to prefer replacing it with >=1.0.0 <2 instead of ^1.0.0. WDYT?

I actually prefer >=1.0.0 <2 over ^1.0.0 and would support using that. It also makes it more consistent with the other larger ranges.

JamieDanielson · 2024-05-22T21:59:31Z

/easycla

trentm · 2024-05-22T22:15:28Z

I actually prefer >=1.0.0 <2 over ^1.0.0

Same.

JamieDanielson

I have similar nits to what Trent pointed out, aside from those LGTM 👍

blumamir added 3 commits May 10, 2024 11:33

feat(amqplib): cap supported versions to 1

9d76835

docs: update guideline with supportedVersions instructions

af9d138

refactor: align all supported version to common format

1602d53

blumamir requested a review from a team as a code owner May 11, 2024 04:55

github-actions bot assigned blumamir, haddasbronfman, JamieDanielson, maryliag, mottibec, naseemkullah, obecny, osherv, pichlermarc and pkanal May 11, 2024

blumamir and others added 18 commits May 15, 2024 09:49

Update GUIDELINES.md

5b5375a

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

eba1e0d

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

096f5c5

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

831c32d

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

b2bafbe

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

87f84dd

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

ea5718a

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Merge remote-tracking branch 'upstream/main' into supported-versions

aaca52d

revert: add back guideline on versioning of redis instrumentations

7f71c9a

Update GUIDELINES.md

3dff4b5

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

1d02804

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

314f302

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update GUIDELINES.md

e02a7c3

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update plugins/node/instrumentation-lru-memoizer/README.md

fc98fcd

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

Update plugins/node/opentelemetry-instrumentation-connect/README.md

7eacab4

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

fix: from code review

4168fea

Merge remote-tracking branch 'origin/supported-versions' into support…

cb72c24

…ed-versions

Merge remote-tracking branch 'upstream/main' into supported-versions

35ab033

blumamir added 2 commits May 18, 2024 09:51

docs: make GUIDELINES shorter

cf866e1

docs: align runtime metrics

d5a4290

github-actions bot added the pkg:instrumentation-runtime-node label May 18, 2024

fix: some fixes from code review

93e2961

blumamir mentioned this pull request May 21, 2024

How to make metadata accessible open-telemetry/opentelemetry-js#4725

Open

trentm approved these changes May 21, 2024

View reviewed changes

JamieDanielson approved these changes May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: align all supported versions to a common format #2196

docs: align all supported versions to a common format #2196

blumamir commented May 11, 2024 •

edited

linux-foundation-easycla bot commented May 18, 2024 •

edited

blumamir commented May 18, 2024

trentm left a comment

trentm May 21, 2024

blumamir May 22, 2024

trentm May 22, 2024

JamieDanielson May 22, 2024

trentm May 21, 2024

trentm May 21, 2024

trentm May 21, 2024

trentm May 21, 2024

JamieDanielson commented May 22, 2024

JamieDanielson commented May 22, 2024

trentm commented May 22, 2024

JamieDanielson left a comment

	- [`graphql`](https://www.npmjs.com/package/graphql) versions `'>=14.0.0 <17'`
	- [`graphql`](https://www.npmjs.com/package/graphql) versions `>=14.0.0 <17`


		Specific guidelines for different cases:

		- For `Different Modules`, instrumentations can use an upper limit on patched packages but it is unknown which future versions of the instrumented package will continue to use it. Thus it is ok to use an open upper limit `>=1.2.3` for the instrumented package.

	- [ ] For instrumentations that use test-all-versions `.tav.yaml`, add the new version to the list of versions to test.
	- [ ] For instrumentations that use test-all-versions `.tav.yml`, add the new version to the list of versions to test.

docs: align all supported versions to a common format #2196

Are you sure you want to change the base?

docs: align all supported versions to a common format #2196

Conversation

blumamir commented May 11, 2024 • edited

linux-foundation-easycla bot commented May 18, 2024 • edited

blumamir commented May 18, 2024

trentm 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

JamieDanielson commented May 22, 2024

JamieDanielson commented May 22, 2024

trentm commented May 22, 2024

JamieDanielson left a comment

Choose a reason for hiding this comment

blumamir commented May 11, 2024 •

edited

linux-foundation-easycla bot commented May 18, 2024 •

edited