decide on advice for use of *unstable* semconv consts: copy or pin dep

[trentm]

[Trent]

I've added that as an option alternative to depending on a pinned version and using the "incubating" entry point. However, do you think we should reverse the suggestion and have the first suggestion be copying the definitions? (This could be done in a separate PR.)

[Jamie]
Thinking through this more... while it may not be a big deal for someone to pin now when only http is stable, it will become a bigger issue when the next wave of stable attributes come through (if there are changes). This would mean folks have the option to either pin and miss out on new stable attributes, or unpin and risk breaking changes. I'm leaning even more heavily now toward copying attributes, but am fine with that as a follow-up.

Originally posted by @JamieDanielson in #5166 (comment)

[trentm]

There is a possibly interesting wrinkle here is the usage of @opentelemetry/semantic-conventions in packages that live in this core repo.

Take this usage, for discussion:

opentelemetry-js/packages/opentelemetry-resources/src/detectors/platform/node/ProcessDetectorSync.ts

Lines 18 to 28 in d8bc6b2

	import {
	SEMRESATTRS_PROCESS_COMMAND,
	SEMRESATTRS_PROCESS_COMMAND_ARGS,
	SEMRESATTRS_PROCESS_EXECUTABLE_NAME,
	SEMRESATTRS_PROCESS_EXECUTABLE_PATH,
	SEMRESATTRS_PROCESS_OWNER,
	SEMRESATTRS_PROCESS_PID,
	SEMRESATTRS_PROCESS_RUNTIME_DESCRIPTION,
	SEMRESATTRS_PROCESS_RUNTIME_NAME,
	SEMRESATTRS_PROCESS_RUNTIME_VERSION,
	} from '@opentelemetry/semantic-conventions';

When doing a semconv package release, current practice in this repo is to update the semconv dep in any other core-repo package to the new semconv package version (pinned). E.g. from this release PR: https://github.com/open-telemetry/opentelemetry-js/pull/5186/files#diff-b12e80a906ecf0de3d9311ef632ab16939846c61c08935857023a117cc3ff910

diff --git a/packages/opentelemetry-resources/package.json b/packages/opentelemetry-resources/package.json
index 6c14650825..79b1796eca 100644
--- a/packages/opentelemetry-resources/package.json
+++ b/packages/opentelemetry-resources/package.json
@@ -92,7 +92,7 @@
   },
   "dependencies": {
     "@opentelemetry/core": "1.28.0",
-    "@opentelemetry/semantic-conventions": "1.27.0"
+    "@opentelemetry/semantic-conventions": "1.28.0"
   },
   "homepage": "https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-resources",
   "sideEffects": false

Currently there is no problem here. However, soonish we'll want to update the deprecated export name usage, e.g. updating from SEMRESATTRS_PROCESS_COMMAND to ATTR_PROCESS_COMMAND, like this:
https://gist.github.com/trentm/f67edf299645d065f6fe2af4ce9ab894

Note how, on line 28 of that diff, we have changed to use the "incubating" entry-point of the semconv package.

Then, the possible issue is that the newer version of @opentelemetry/semantic-conventions could break unstable semconv exports in a minor release.

Then, the question is:

1. For core-repo packages that use unstable semconv exports, should we be copying/duplicating the semconv constants and not use the "incubating" entry-point?
2. Or should we change the semconv package release process to (manually) work through possible breakages in the "incubating" entry-point from the new version of the semconv package?

[trentm]

Then, the question is:

#5187 is a draft PR that shows both options, using the "resources" package as a demo.

[dyladan]

When a key is removed it should be moved to the deprecated.yml in semconv. On next release it should still be included in code generation, just with a @deprecated tag. I think there's no reason to pin the dependency. I'd say it even does us a favor by letting linters tell us which packages are out of date in semconv.

[trentm]

I think there's no reason to pin the dependency.

Then why do the semconv incubating packages/entry-points (for JS, Java, etc.) state that the incubating package can have breaking changes in minor versions?

Are you positive that some experimental semconv keys don't just get removed rather than moved to "deprecated.yml"?

[trentm]

Are you positive that some experimental semconv keys don't just get removed rather than moved to "deprecated.yml"?

I briefly tried to find a counter example (i.e. a breaking change going from 1.27.0 to 1.28.0) and could not find one.

Do you happen to know if the JS semconv package is different from the other langs in someway that we don't have breaking changes in "incubating"? Perhaps the other langs don't emit deprecated keys?

So I wonder if it is fine for us to remove the mention here of the incubating entry-point having breaking changes and users needed to beware how they use it.

[dyladan]

Perhaps the other langs don't emit deprecated keys?

Some languages do not emit deprecated keys. At least go used to be like that.

[trentm]

A side-effect of pinning in the contrib repo:

open-telemetry/opentelemetry-js-contrib#2473 pinned the semconv dep for instrumentation-pg "as it's using the incubating entrypoint, which may break on minor bumps."
That is currently the only package in the contrib repo that is pinning the semconv dep.
The result of that is that the scripts/update-otel-deps.js script now fails to automatically update @opentelemetry/semantic-conventions from 1.27.0 to 1.28.0.

... as it should fail, because: it fails because the dep is pinned, presumably because incubating is being used, which could have a breaking change. So, in general, it requires a manual look to check for those breaking changes.

We'll need to change the "update otel deps" process somehow to accommodate this. I'll look into it.

[trentm]

Maintainers discussed this earlier today. My notes:

• We decided to recommend pinning the semconv dep if using the "incubating" entry-point.
  • However, a concern (as express in Jamie's comment in this issue description) is that a package using both stable and unstable semconv will need to pin. When that package wants to use newly-stabilized semconv exports the update is slightly harder because they need to watch out for possibly breaking changes in the exports being used from the "incubating" endpoint.
  • One of the reasons the JS advice (whether to pin the dep or make a copy of any used unstable exports) will differ from Java's (and I assume Python's) is that their import mechanisms do not allow having multiple version of the semantic-conventions package installed. JS's does allow that, so we can use the pinning option.
  • TODO: update the semconv/README.md usage notes (https://github.com/open-telemetry/opentelemetry-js/blob/main/semantic-conventions/README.md#usage) accordingly
• When a user of the JS semconv package is updating, they need advice on how to do so. Ideally we need better changelog/release notes for this package. To a degree the semconv repo changelog can be this changelog, but it might be too obtuse/dense for users of the JS package.
  • what breaking changes, if any, are in the "incubating" package for this version?
  • what's new in the stable exports?
  • what's new in the incubating package?

Some examples of breaking changes in the incubating entry-point we've seen or expect:

• In the 1.28.0 release there were some changes in the values of db.cosmosdb.operation.type E.g.:

-export const DB_COSMOSDB_OPERATION_TYPE_VALUE_EXECUTE = "Execute" as const;
+export const DB_COSMOSDB_OPERATION_TYPE_VALUE_EXECUTE = "execute" as const;
-export const DB_COSMOSDB_OPERATION_TYPE_VALUE_EXECUTE_JAVASCRIPT = "ExecuteJavaScript" as const;
+export const DB_COSMOSDB_OPERATION_TYPE_VALUE_EXECUTE_JAVASCRIPT = "execute_javascript" as const;

• In the future, there will likely be an exception to the semconv "no change in only underscore or dots" rule to allow changing from export const ATTR_FEATURE_FLAG_PROVIDER_NAME = 'feature_flag.provider_name'; to feature_flag.provider.name.

[trentm]

open-telemetry/opentelemetry-js-contrib#2569 adds a 'npm run lint' step to ensure the semconv dep is pinned if a package is using the incubating entry-point (only usage in "src/**/*.ts" is checked).