Skip to content

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Nov 30, 2024

This adds a lint step that checks that packages in the workspace
are following our "rules" on usage of '@opentelemetry/semantic-conventions'
dep: don't pin it, don't use the 'incubating' entry point.
https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv

Refs: #2549 (comment)
Refs: open-telemetry/opentelemetry-js#5182

checklist

This adds a lint step that checks that packages in the workspace
are following our 'rule' that uses of the semconv pkg 'incubating'
entry-point should *pin* the '@opentelemetry/semantic-conventions'
dep. This is because (though rare) the incubating/unstable
semconv exports can have breaking changes in minors.

Refs: open-telemetry#2549 (comment)
Refs: open-telemetry/opentelemetry-js#5182
@trentm trentm self-assigned this Nov 30, 2024
@trentm trentm requested a review from a team as a code owner November 30, 2024 00:58
@trentm
Copy link
Contributor Author

trentm commented Nov 30, 2024

Currently there is one package that is breaking this rule:

% npm run lint:semconv-deps

> [email protected] lint:semconv-deps
> ./scripts/lint-semconv-deps.mjs

lint-semconv-deps error: package @opentelemetry/instrumentation-aws-lambda (in plugins/node/opentelemetry-instrumentation-aws-lambda) imports "@opentelemetry/semantic-conventions/incubating" but does not *pin* the dependency: `"@opentelemetry/semantic-conventions": "^1.27.0"`

% echo $?
1

I'll update that dep in a commit on this PR.

@trentm trentm changed the title chore: add 'lint:semconv-deps' fix(instrumentation-aws-lambda): add 'lint:semconv-deps' Nov 30, 2024
@codecov
Copy link

codecov bot commented Nov 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.40%. Comparing base (58bc158) to head (9fbca7f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2569   +/-   ##
=======================================
  Coverage   92.40%   92.40%           
=======================================
  Files         171      171           
  Lines        8142     8142           
  Branches     1647     1647           
=======================================
  Hits         7524     7524           
  Misses        618      618           

Copy link
Contributor

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should instrumentation-aws-lambda be removed from commit message since the script is global?

@trentm
Copy link
Contributor Author

trentm commented Dec 3, 2024

Should instrumentation-aws-lambda be removed from commit message since the script is global?

This includes a change to instrumentation-aws-lambda's package.json#dependencies for which there should be a new version of this package, so I think it should stay. IIRC keeping it in the commit title is what gets the release process to notice that there should be a new version of this package.

Copy link
Contributor

@jj22ee jj22ee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving update to instrumentation-aws-lambda's package.json.

@trentm
Copy link
Contributor Author

trentm commented Dec 12, 2024

Given the decision here: open-telemetry/opentelemetry-js#5182 (comment)
(and the coming docs for that: open-telemetry/opentelemetry-js#5256), I'll be changing this PR so that the linting breaks if the semconv 'incubating' is used at all in runtime code.

@trentm trentm marked this pull request as draft December 12, 2024 01:43
@trentm
Copy link
Contributor Author

trentm commented Jan 17, 2025

Okay, I've switch over the linting behaviour to the final guidance from open-telemetry/opentelemetry-js#5182 (comment)

In the current repo state there are these lint failures:

% npm run lint:semconv-deps

> [email protected] lint:semconv-deps
> ./scripts/lint-semconv-deps.mjs

lint-semconv-deps error: plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)
lint-semconv-deps error: detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsLambdaDetectorSync.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)
lint-semconv-deps error: detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEksDetectorSync.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)
lint-semconv-deps error: detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetectorSync.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)
lint-semconv-deps error: detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEc2DetectorSync.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)
lint-semconv-deps error: detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsBeanstalkDetectorSync.ts: uses the 'incubating' entry-point from '@opentelemetry/semantic-conventions', but should not (see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv)

Those were added recently in #2642. I'll open a PR to fix those. That one should be merged before this one.

@trentm
Copy link
Contributor Author

trentm commented Jan 17, 2025

Those were added recently in #2642. I'll open a PR to fix those. That one should be merged before this one.

Ah, Gary beat me to it with: #2668

@trentm trentm marked this pull request as ready for review January 18, 2025 01:11
@trentm trentm requested review from jj22ee and trivikr January 18, 2025 01:11
@pichlermarc pichlermarc added the has:owner-approval Approved by Component Owner label Jan 20, 2025
@trentm trentm changed the title fix(instrumentation-aws-lambda): add 'lint:semconv-deps' chore: add 'lint:semconv-deps' Jan 20, 2025
@trentm
Copy link
Contributor Author

trentm commented Jan 20, 2025

I'm not sure why I had added the fix(instrumentation-aws-lambda): prefix to this PR before. I think long ago it may have included some fixes to that instruemtnation that have since been pulled out.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍

@pichlermarc pichlermarc merged commit 7a1e8b3 into open-telemetry:main Feb 11, 2025
12 checks passed
trentm added a commit to trentm/opentelemetry-js that referenced this pull request Feb 11, 2025
This adds the same lint step added to the contrib repo in
open-telemetry/opentelemetry-js-contrib#2569
It ensures that the semantic-conventions dep is being used
as recommended: no usage of incubating, no pinning of the dep.
@trentm trentm deleted the tm-semconv-lint-and-changelog-play branch February 11, 2025 23:47
deejay1 pushed a commit to deejay1/opentelemetry-js-contrib that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants