ARSN-601: Promote OTEL SDK packages from optionalDependencies to dependencies#2653
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
f24f840 to
dc9eb23
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development/8.4 #2653 +/- ##
===================================================
+ Coverage 74.00% 74.03% +0.02%
===================================================
Files 229 229
Lines 18503 18503
Branches 3820 3845 +25
===================================================
+ Hits 13694 13699 +5
+ Misses 4804 4799 -5
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue ARSN-601. Goodbye delthas. The following options are set: approve |
Summary
Move the four OTEL SDK packages —
@opentelemetry/sdk-node,sdk-trace-base,resources,exporter-trace-otlp-http— fromoptionalDependenciestodependencies.ioctlstays optional.Problem
ARSN-586 classified the SDK packages as
optionalDependencies, intending to mean "consumers who don't use tracing skip the ~20 MB install cost". That misuses the npm semantic.optionalDependenciesis for packages that may fail to install (typically native binaries on unsupported platforms) and whose absence the calling code tolerates gracefully. Canonical examples:fsevents,ioctl.lib/tracing/bootstrap.ts:41does:A bare
require(). If the SDK is missing, the process crashes — that's regulardependenciessemantics, not optional.The symptom surfaced via CLDSRV-937: cloudserver's production Dockerfile uses
yarn install --ignore-optional(added in 2019 during the npm→yarn migration with no commit-message rationale, never revisited). Images built that way ship without the SDK, then crash at startup whenENABLE_OTEL=true:Thanks to @DarkIsDude for flagging the optional-dep misclassification in CLDSRV-937's review.
Fix
"dependencies": { + "@opentelemetry/exporter-trace-otlp-http": "^0.219.0", + "@opentelemetry/resources": "^2.8.0", + "@opentelemetry/sdk-node": "^0.219.0", + "@opentelemetry/sdk-trace-base": "^2.8.0", ... }, "optionalDependencies": { - "@opentelemetry/exporter-trace-otlp-http": "^0.219.0", - "@opentelemetry/resources": "^2.8.0", - "@opentelemetry/sdk-node": "^0.219.0", - "@opentelemetry/sdk-trace-base": "^2.8.0", "ioctl": "^2.0.2" }ioctlis the only remainingoptionalDependenciesentry — its usage atlib/storage/utils.js:46-51is wrapped intry/catchwith a warning fallback, so the optional semantic is honored.lib/tracing/README.mdis updated to reflect the new classification with the rationale.Precedent in arsenal
Arsenal already ships packages as regular
dependenciesthat not every consumer uses:mongodb— some deployments use bucketd, not mongoioredis— some consumers don't use redissocket.io,sproxydclient,@aws-sdk/*There's no in-tree precedent for "opt-in feature SDK as
optionalDependencies" — that was a novel (and incorrect) classification introduced by ARSN-586.Why not the alternatives
ENABLE_OTEL=trueand SDK is missing. Worse UX — operator explicitly asked for tracing; fail-fast is preferable. Also doesn't address--ignore-optional— just papers over it.package.json: doesn't isolate anything (other arsenal consumers without--ignore-optionalalready pull them in), adds friction for every future tracing consumer.peerDependencieswithoptional: true: arsenal is git-pinned in consumers via yarn 1, which has rough handling of peer deps for git URLs. Not pragmatic.Cost
~20 MB to backbeat / vault / sproxyd installs. Acceptable — backbeat already consumes arsenal's
kafka.*tracing helpers, and broader OTEL adoption is the direction the stack is moving.Verification
Sandbox test (
yarn install --ignore-optionalagainst this branch viafile:dep):node_modules/@opentelemetry/sdk-nodepresent ✓ (the fix)node_modules/ioctlabsent ✓ (still optional, no regression)Plus locally:
yarn build,yarn lint,yarn jest tests/unit/tracing/all green (71/71).Relates to: ARSN-586, CLDSRV-937
Issue: ARSN-601