Skip to content

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Jan 18, 2025

Refs: https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv


Now that we've established how we want packages to use unstable semconv definitions, we are starting to get PRs adding a local copy of these: #2664, #2668

It is helpful to script this to: make it faster, encourage a common pattern for these local copies, avoid cut 'n paste errors.

An example run of this (for #2668) looks like this:

% ./scripts/gen-semconv-ts.js detectors/node/opentelemetry-resource-detector-aws
Found import of 31 unstable semconv definitions.
Generated "detectors/node/opentelemetry-resource-detector-aws/src/semconv.ts".
Running 'npx eslint --fix src/semconv.ts' to fix formatting.

It will also warn/error on attempts to put stable semconv exports in there. That looks like this:

% ../../../scripts/gen-semconv-ts.js
gen-semconv-ts warning: ./src/detectors/AwsBeanstalkDetectorSync.ts: 'ATTR_SERVICE_NAME' export is available on the stable "@opentelemetry/semantic-conventions" entry-point. This definition will not be included in the generated semconv.ts. Instead use:
    import { ATTR_SERVICE_NAME } from '@opentelemetry/semantic-conventions';
gen-semconv-ts warning: ./src/detectors/AwsBeanstalkDetectorSync.ts: 'ATTR_SERVICE_VERSION' export is available on the stable "@opentelemetry/semantic-conventions" entry-point. This definition will not be included in the generated semconv.ts. Instead use:
    import { ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions';
Found import of 31 unstable semconv definitions.
Generated "src/semconv.ts".

@trentm trentm self-assigned this Jan 18, 2025
@trentm trentm requested a review from a team as a code owner January 18, 2025 00:54
trentm added a commit to garysassano/opentelemetry-js-contrib that referenced this pull request Jan 18, 2025
@codecov
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.41%. Comparing base (d2a9a20) to head (3484f6e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2669      +/-   ##
==========================================
- Coverage   92.46%   92.41%   -0.05%     
==========================================
  Files         171      171              
  Lines        8150     8150              
  Branches     1653     1653              
==========================================
- Hits         7536     7532       -4     
- Misses        614      618       +4     

see 1 file with indirect coverage changes

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, that's pretty cool 🙂

I've tried it out and it seems that it also includes the @experimental note about how breaking changes in the semconv package would affect it. Not sure if it would make sense to replace those with an empty string or something like "generated from experimental semconv". 🤔

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

I've tried it out and it seems that it also includes the @experimental note about how breaking changes in the semconv package would affect it.

Looking at an example (for experimental/packages/web-common):

/**
 * A unique id to identify a session.
 *
 * @example "00112233-4455-6677-8899-aabbccddeeff"
 *
 * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`.
 */
export const ATTR_SESSION_ID = 'session.id';

It is still accurate, no? I.e. that it could break in @opentelemetry/semantic-conventions.
I guess a maintainer could be confused, given that the copied var is no longer in that package.

The generated top-comment:

/*
 * This file contains a copy of unstable semantic convention definitions
 * used by this package.
 * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
 */

is meant to help give context.

I think it could be helpful to the maintainer/reader to know that the const is still experimental in semconv.

Thoughts?

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

Not sure if it would make sense to replace those with an empty string or something like "generated from experimental semconv".

Or even include the semconv version in that sentence. 🤔

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

@pichlermarc @dyladan Separate question: the semconv constants have as const;. Currently this script does not add that (mainly because it is getting the content from the local non-TS build/esnext/experimental_*.js files). What does the as const; accomplish?

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

What does the as const; accomplish?

Answering myself: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions

…an build/esnext/... sources in a local npm install.

This does a shallow git clone if necessary.
@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

Commit c058095 adds the as const suffix if it is in the TypeScript sources. It does this by using the TS sources now, instead of build/esnext/*.js files in a local npm install.

@pichlermarc
Copy link
Member

The generated top-comment:

/*
 * This file contains a copy of unstable semantic convention definitions
 * used by this package.
 * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
 */

is meant to help give context.

I think it could be helpful to the maintainer/reader to know that the const is still experimental in semconv.

Thoughts?

Ah, sorry I did not see that one. All good 👍

@trentm trentm enabled auto-merge (squash) February 20, 2025 22:32
@trentm trentm merged commit 3fc62be into open-telemetry:main Feb 20, 2025
4 checks passed
@trentm trentm deleted the tm-gen-semconv-ts branch February 20, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants