From 338df24a33996d653c98bb7aecf864d4c1cf84a4 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 15:20:40 +0200 Subject: [PATCH 1/8] chore: Add vendor-otel skill for OTel instrumentation vendoring Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 168 ++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 .agents/skills/vendor-otel/SKILL.md diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md new file mode 100644 index 000000000000..a83710bc2a24 --- /dev/null +++ b/.agents/skills/vendor-otel/SKILL.md @@ -0,0 +1,168 @@ +--- +name: vendor-otel +description: Vendor an OpenTelemetry instrumentation package into the Sentry JavaScript SDK. Use when vendoring, inlining, or copying an @opentelemetry/instrumentation-* package (or similar like @prisma/instrumentation, @fastify/otel) into the SDK source. Also use when the user says "vendor", "inline instrumentation", or references the Vendor OpenTelemetry Instrumentation project. +--- + +# Vendor OTel Instrumentation + +Copy upstream OTel instrumentation TypeScript source into a `vendored/` directory, remove the npm dependency, and ensure builds and tests pass. No logic changes — the vendored code must behave identically to the original. + +## 1. Research + +### Find upstream source +```bash +gh api "repos/open-telemetry/opentelemetry-js-contrib/git/trees/main?recursive=1" --jq '.tree[].path' | grep "instrumentation-/src/.*\.ts$" +``` + +### Check versions +- Pinned version: `grep "instrumentation-" packages/node/package.json` +- Latest tag: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags --jq '.[].ref' | grep "instrumentation-"` +- Get commit SHA: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags/instrumentation--v --jq '.object.sha'` + +### Verify no breaking changes between pinned and latest +**This is critical.** All OTel instrumentations are pre-v1 so any bump could break things. Diff ALL source files: +```bash +diff <(gh api ".../src/?ref=instrumentation--v" --jq '.content' | base64 -d) \ + <(gh api ".../src/?ref=instrumentation--v" --jq '.content' | base64 -d) +``` +In practice most bumps have zero code changes (only license headers). If there ARE changes, evaluate safety and report to the user before proceeding. + +### Check external type imports +```bash +grep "import.*from '" | grep -v "@opentelemetry\|'\./\|@sentry\|'util'\|'path'\|'fs'\|'http'\|'events'" +``` +External type imports need special handling (see section 4). + +### Check test coverage +- Integration tests: `dev-packages/node-integration-tests/suites/tracing//` +- E2E tests: `dev-packages/e2e-tests/test-applications/node-/` +- Unit tests: `packages/node/test/integrations/tracing/.test.ts` + +Report any gaps to the user (e.g., "no integration test exists for this instrumentation" or "tests exist but don't cover X functionality"). + +## 2. Directory Structure + +Move the integration file into a directory: +- `packages/node/src/integrations/tracing/.ts` → `packages/node/src/integrations/tracing//index.ts` +- For non-tracing integrations (like `fs.ts`): `packages/node/src/integrations//index.ts` +- For non-node packages (aws-serverless, nestjs): follow their existing structure +- Create `vendored/` subdirectory for upstream files + +Import paths in barrel exports (`index.ts`, `tracing/index.ts`) resolve to the directory's `index.ts` automatically — usually no changes needed. + +## 3. Vendoring Source Files + +Fetch original TypeScript from the OTel contrib repo (NOT compiled JS from node_modules): +```bash +gh api "repos/open-telemetry/opentelemetry-js-contrib/contents/?ref=" --jq '.content' | base64 -d +``` + +**Be careful with header stripping** — `sed '1,Nd'` to remove the SPDX header can accidentally strip import lines. Always verify all imports are present after stripping. + +Each vendored file gets this header: +``` +/* + * Copyright The OpenTelemetry Authors + * ...full Apache 2.0 license text... + * + * NOTICE from the Sentry authors: + * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree//packages/instrumentation- + * - Upstream version: @opentelemetry/instrumentation-@ + */ +/* eslint-disable */ +``` + +Add additional bullets to the NOTICE only when applicable: +- `* - Minor TypeScript strictness adjustments for this repository's compiler settings` — when TS changes were made +- `* - Some types vendored from with simplifications` — when types were inlined + +Standard replacements in the main instrumentation file: +- Remove `import { PACKAGE_NAME, PACKAGE_VERSION } from './version'` and the `/** @knipignore */` comment above it +- Add `import { SDK_VERSION } from '@sentry/core'` +- Add `const PACKAGE_NAME = '@sentry/instrumentation-';` +- Replace `PACKAGE_VERSION` with `SDK_VERSION` in the `super()` call + +Don't forget barrel exports (`enums/index.ts`, etc.) if the upstream has them. + +## 4. External Type Handling + +Types from external packages can leak into `.d.ts` output and break consumers. + +**Decision tree:** +1. Check if types appear in public class signatures. TypeScript strips private method signatures from `.d.ts`, so private-only usage won't leak. +2. Check if the upstream `types.ts` already vendors some types inline. +3. If types leak or are needed for compilation, **inline simplified types**: + - Put in a separate `-types.ts` file in vendored/ + - Keep as close to originals as possible — same generic parameters, field names, types + - Only simplify when the full type tree is too deep + - Add `[key: string]: any` index signatures for permissiveness + - Check if package ships own types or uses DefinitelyTyped + +4. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` + +## 5. TypeScript Strictness Adjustments + +The Sentry repo has `strict: true` and `noUncheckedIndexedAccess: true`. Common fixes: +- `` angle-bracket assertions → `as any` (sucrase/esbuild doesn't support angle-bracket syntax) +- Implicit `any` in `.then()`, `.catch()`, `.forEach()`, `.map()` callbacks → add `: any` +- Array indexing `T | undefined` → add `!` non-null assertion or default values +- `ConstructorParameters` constraint failures → replace with `any[]` + +Add the TS strictness bullet to the header comment when changes are made. + +## 6. Package.json and Lint Config + +- Remove the dependency from the relevant `package.json` +- If vendored code imports `@opentelemetry/core` and the package didn't previously have it as a direct dependency, **add it** — rollup auto-externalizes based on `dependencies`, without it the import resolves to a broken relative path +- Add vendored path to the consolidated lint exceptions in `.oxlintrc.base.json`: +```json +"**/integrations/tracing//vendored/**/*.ts" +``` + +## 7. Build and Format + +```bash +yarn install +npx oxfmt --write / +yarn build:dev:filter @sentry/ +``` + +Check `.d.ts` output for type leaks. For `aws-serverless`: needs `preserveModulesRoot: 'src'` in rollup config. + +## 8. Test Verification + +- Run integration tests: `cd dev-packages/node-integration-tests && yarn test suites/tracing/` +- Run unit tests: `cd packages/node && yarn test:unit test/integrations/tracing/.test.ts` +- **Update test imports**: unit tests importing from `@opentelemetry/instrumentation-` need paths updated to the vendored location, and `vi.mock()` calls updated to match +- Docker-dependent tests (amqplib, kafkajs, redis, postgres) timeout locally but pass in CI +- Version-specific tests: use a local `package.json` in the test directory with unique Docker container names + +**Report test coverage gaps to the user** — if an instrumentation has no integration test, no E2E test, or tests don't cover key functionality, flag it. + +## 9. Report Changes + +**Before submitting, report ALL modifications to the user for verification:** + +1. **Files copied as-is** (only header + formatting changes) — list them +2. **TypeScript strictness adjustments** — list each change with file and line context (e.g., "added `: any` to `.catch()` callback parameter") +3. **Type simplifications** — explain what was simplified vs the original and why +4. **Import path changes** — `from 'kafkajs'` → `from './kafkajs-types'` etc. +5. **Any other modifications** — anything beyond the standard vendoring pattern + +The user should be able to verify that no logic was changed and all adjustments were strictly necessary. + +## 10. PR Creation + +- Branch: `nh/vendor--instrumentation` +- Commit: `ref(node): Vendor instrumentation` +- PR body: one sentence on what was vendored, mention inlined types if applicable, reference closing issue +- Always draft PR, base branch `develop` + +## 11. Common Pitfalls + +- `sed` header stripping removing import lines — always verify +- `preserveModules: true` shifting output paths with deeply nested vendored files +- Docker container name conflicts between test suites +- `/* eslint-disable */` kept for consistency even though project uses oxlint +- `yarn.lock` duplicates — run `npx yarn-deduplicate yarn.lock` +- `.oxlintrc.base.json` merge conflicts — keep both HEAD and develop entries From d5795fcc21bb09f3d0f3d9f4349c3c8cd34b7758 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 15:49:39 +0200 Subject: [PATCH 2/8] chore: Trim vendor-otel skill and add plan-first step Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 136 +++++++++++----------------- 1 file changed, 51 insertions(+), 85 deletions(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index a83710bc2a24..4b334c9e5687 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -5,164 +5,130 @@ description: Vendor an OpenTelemetry instrumentation package into the Sentry Jav # Vendor OTel Instrumentation +**Input:** The npm package name to vendor (e.g., `@opentelemetry/instrumentation-graphql`). + Copy upstream OTel instrumentation TypeScript source into a `vendored/` directory, remove the npm dependency, and ensure builds and tests pass. No logic changes — the vendored code must behave identically to the original. ## 1. Research -### Find upstream source +Find upstream source files: ```bash gh api "repos/open-telemetry/opentelemetry-js-contrib/git/trees/main?recursive=1" --jq '.tree[].path' | grep "instrumentation-/src/.*\.ts$" ``` -### Check versions -- Pinned version: `grep "instrumentation-" packages/node/package.json` +Check versions: +- Pinned: `grep "instrumentation-" packages/node/package.json` - Latest tag: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags --jq '.[].ref' | grep "instrumentation-"` -- Get commit SHA: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags/instrumentation--v --jq '.object.sha'` +- Commit SHA: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags/instrumentation--v --jq '.object.sha'` -### Verify no breaking changes between pinned and latest -**This is critical.** All OTel instrumentations are pre-v1 so any bump could break things. Diff ALL source files: -```bash -diff <(gh api ".../src/?ref=instrumentation--v" --jq '.content' | base64 -d) \ - <(gh api ".../src/?ref=instrumentation--v" --jq '.content' | base64 -d) -``` -In practice most bumps have zero code changes (only license headers). If there ARE changes, evaluate safety and report to the user before proceeding. +**Diff ALL source files between pinned and latest version.** All OTel instrumentations are pre-v1 so any bump could introduce breaking changes. Report findings to the user. -### Check external type imports +Check for external type imports (these need special handling, see section 5): ```bash grep "import.*from '" | grep -v "@opentelemetry\|'\./\|@sentry\|'util'\|'path'\|'fs'\|'http'\|'events'" ``` -External type imports need special handling (see section 4). -### Check test coverage +Check test coverage and report gaps: - Integration tests: `dev-packages/node-integration-tests/suites/tracing//` - E2E tests: `dev-packages/e2e-tests/test-applications/node-/` - Unit tests: `packages/node/test/integrations/tracing/.test.ts` -Report any gaps to the user (e.g., "no integration test exists for this instrumentation" or "tests exist but don't cover X functionality"). +## 2. Plan + +Present a plan to the user covering: +- Which version to vendor (pinned vs latest, with diff summary) +- Source files to copy +- External types that need inlining +- Test coverage status +- Any concerns -## 2. Directory Structure +Wait for user approval before implementing. + +## 3. Directory Structure -Move the integration file into a directory: - `packages/node/src/integrations/tracing/.ts` → `packages/node/src/integrations/tracing//index.ts` - For non-tracing integrations (like `fs.ts`): `packages/node/src/integrations//index.ts` - For non-node packages (aws-serverless, nestjs): follow their existing structure - Create `vendored/` subdirectory for upstream files -Import paths in barrel exports (`index.ts`, `tracing/index.ts`) resolve to the directory's `index.ts` automatically — usually no changes needed. - -## 3. Vendoring Source Files +## 4. Vendor Source Files -Fetch original TypeScript from the OTel contrib repo (NOT compiled JS from node_modules): +Fetch original TypeScript from the OTel contrib GitHub repo (NOT compiled JS from node_modules): ```bash gh api "repos/open-telemetry/opentelemetry-js-contrib/contents/?ref=" --jq '.content' | base64 -d ``` -**Be careful with header stripping** — `sed '1,Nd'` to remove the SPDX header can accidentally strip import lines. Always verify all imports are present after stripping. +When stripping the upstream SPDX header, verify all import lines are still present afterward. -Each vendored file gets this header: +Each vendored file gets the full Apache 2.0 license header plus: ``` -/* - * Copyright The OpenTelemetry Authors - * ...full Apache 2.0 license text... - * * NOTICE from the Sentry authors: * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree//packages/instrumentation- * - Upstream version: @opentelemetry/instrumentation-@ - */ -/* eslint-disable */ ``` +Add bullets for TS adjustments or type vendoring only when applicable. -Add additional bullets to the NOTICE only when applicable: -- `* - Minor TypeScript strictness adjustments for this repository's compiler settings` — when TS changes were made -- `* - Some types vendored from with simplifications` — when types were inlined +Append `/* eslint-disable */` after the header block. Standard replacements in the main instrumentation file: -- Remove `import { PACKAGE_NAME, PACKAGE_VERSION } from './version'` and the `/** @knipignore */` comment above it -- Add `import { SDK_VERSION } from '@sentry/core'` -- Add `const PACKAGE_NAME = '@sentry/instrumentation-';` +- Remove `import { PACKAGE_NAME, PACKAGE_VERSION } from './version'` +- Add `import { SDK_VERSION } from '@sentry/core'` and `const PACKAGE_NAME = '@sentry/instrumentation-';` - Replace `PACKAGE_VERSION` with `SDK_VERSION` in the `super()` call -Don't forget barrel exports (`enums/index.ts`, etc.) if the upstream has them. +Include barrel exports (`enums/index.ts`, etc.) if the upstream has them. -## 4. External Type Handling +## 5. External Type Handling Types from external packages can leak into `.d.ts` output and break consumers. -**Decision tree:** -1. Check if types appear in public class signatures. TypeScript strips private method signatures from `.d.ts`, so private-only usage won't leak. +1. Check if types appear in public class signatures (private-only usage won't leak to `.d.ts`). 2. Check if the upstream `types.ts` already vendors some types inline. 3. If types leak or are needed for compilation, **inline simplified types**: - Put in a separate `-types.ts` file in vendored/ - Keep as close to originals as possible — same generic parameters, field names, types - Only simplify when the full type tree is too deep - Add `[key: string]: any` index signatures for permissiveness - - Check if package ships own types or uses DefinitelyTyped - 4. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` -## 5. TypeScript Strictness Adjustments +## 6. TypeScript Adjustments -The Sentry repo has `strict: true` and `noUncheckedIndexedAccess: true`. Common fixes: -- `` angle-bracket assertions → `as any` (sucrase/esbuild doesn't support angle-bracket syntax) -- Implicit `any` in `.then()`, `.catch()`, `.forEach()`, `.map()` callbacks → add `: any` -- Array indexing `T | undefined` → add `!` non-null assertion or default values -- `ConstructorParameters` constraint failures → replace with `any[]` +Fix any compilation errors caused by this repository's strict TypeScript settings (`strict: true`, `noUncheckedIndexedAccess: true`). Add a `Minor TypeScript strictness adjustments` bullet to the header when changes are made. -Add the TS strictness bullet to the header comment when changes are made. - -## 6. Package.json and Lint Config +## 7. Package.json and Lint Config - Remove the dependency from the relevant `package.json` -- If vendored code imports `@opentelemetry/core` and the package didn't previously have it as a direct dependency, **add it** — rollup auto-externalizes based on `dependencies`, without it the import resolves to a broken relative path -- Add vendored path to the consolidated lint exceptions in `.oxlintrc.base.json`: -```json -"**/integrations/tracing//vendored/**/*.ts" -``` +- If vendored code imports a package (e.g., `@opentelemetry/core`) that isn't a direct dependency, add it — rollup auto-externalizes based on `dependencies` +- Add vendored path to the consolidated lint exceptions in `.oxlintrc.base.json` -## 7. Build and Format +## 8. Build, Format, and Test ```bash yarn install -npx oxfmt --write / +yarn fix yarn build:dev:filter @sentry/ ``` -Check `.d.ts` output for type leaks. For `aws-serverless`: needs `preserveModulesRoot: 'src'` in rollup config. - -## 8. Test Verification +Verify no external types leak into `.d.ts` output. -- Run integration tests: `cd dev-packages/node-integration-tests && yarn test suites/tracing/` -- Run unit tests: `cd packages/node && yarn test:unit test/integrations/tracing/.test.ts` -- **Update test imports**: unit tests importing from `@opentelemetry/instrumentation-` need paths updated to the vendored location, and `vi.mock()` calls updated to match -- Docker-dependent tests (amqplib, kafkajs, redis, postgres) timeout locally but pass in CI -- Version-specific tests: use a local `package.json` in the test directory with unique Docker container names +Run existing tests: +- `cd dev-packages/node-integration-tests && yarn test suites/tracing/` +- `cd packages/node && yarn test:unit test/integrations/tracing/.test.ts` -**Report test coverage gaps to the user** — if an instrumentation has no integration test, no E2E test, or tests don't cover key functionality, flag it. +Update unit test imports from `@opentelemetry/instrumentation-` to the vendored path, including `vi.mock()` calls. ## 9. Report Changes -**Before submitting, report ALL modifications to the user for verification:** +Before submitting, report ALL modifications to the user: -1. **Files copied as-is** (only header + formatting changes) — list them -2. **TypeScript strictness adjustments** — list each change with file and line context (e.g., "added `: any` to `.catch()` callback parameter") -3. **Type simplifications** — explain what was simplified vs the original and why -4. **Import path changes** — `from 'kafkajs'` → `from './kafkajs-types'` etc. -5. **Any other modifications** — anything beyond the standard vendoring pattern - -The user should be able to verify that no logic was changed and all adjustments were strictly necessary. +1. **Files copied as-is** (only header + formatting) +2. **TypeScript adjustments** — each change with file and line context +3. **Type simplifications** — what was simplified and why +4. **Import path changes** +5. **Any other modifications** ## 10. PR Creation -- Branch: `nh/vendor--instrumentation` +- Branch: `vendor--instrumentation` - Commit: `ref(node): Vendor instrumentation` -- PR body: one sentence on what was vendored, mention inlined types if applicable, reference closing issue +- PR body: one sentence describing what was vendored, mention inlined types if applicable, reference closing issue - Always draft PR, base branch `develop` - -## 11. Common Pitfalls - -- `sed` header stripping removing import lines — always verify -- `preserveModules: true` shifting output paths with deeply nested vendored files -- Docker container name conflicts between test suites -- `/* eslint-disable */` kept for consistency even though project uses oxlint -- `yarn.lock` duplicates — run `npx yarn-deduplicate yarn.lock` -- `.oxlintrc.base.json` merge conflicts — keep both HEAD and develop entries From 5a6cf8c080bc06233d32bc652a785cfdff389025 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 15:51:22 +0200 Subject: [PATCH 3/8] yarn fix --- .agents/skills/vendor-otel/SKILL.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index 4b334c9e5687..f9c94a3ce6d5 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -12,11 +12,13 @@ Copy upstream OTel instrumentation TypeScript source into a `vendored/` director ## 1. Research Find upstream source files: + ```bash gh api "repos/open-telemetry/opentelemetry-js-contrib/git/trees/main?recursive=1" --jq '.tree[].path' | grep "instrumentation-/src/.*\.ts$" ``` Check versions: + - Pinned: `grep "instrumentation-" packages/node/package.json` - Latest tag: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags --jq '.[].ref' | grep "instrumentation-"` - Commit SHA: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags/instrumentation--v --jq '.object.sha'` @@ -24,11 +26,13 @@ Check versions: **Diff ALL source files between pinned and latest version.** All OTel instrumentations are pre-v1 so any bump could introduce breaking changes. Report findings to the user. Check for external type imports (these need special handling, see section 5): + ```bash grep "import.*from '" | grep -v "@opentelemetry\|'\./\|@sentry\|'util'\|'path'\|'fs'\|'http'\|'events'" ``` Check test coverage and report gaps: + - Integration tests: `dev-packages/node-integration-tests/suites/tracing//` - E2E tests: `dev-packages/e2e-tests/test-applications/node-/` - Unit tests: `packages/node/test/integrations/tracing/.test.ts` @@ -36,6 +40,7 @@ Check test coverage and report gaps: ## 2. Plan Present a plan to the user covering: + - Which version to vendor (pinned vs latest, with diff summary) - Source files to copy - External types that need inlining @@ -54,6 +59,7 @@ Wait for user approval before implementing. ## 4. Vendor Source Files Fetch original TypeScript from the OTel contrib GitHub repo (NOT compiled JS from node_modules): + ```bash gh api "repos/open-telemetry/opentelemetry-js-contrib/contents/?ref=" --jq '.content' | base64 -d ``` @@ -61,16 +67,19 @@ gh api "repos/open-telemetry/opentelemetry-js-contrib/contents/?ref=" When stripping the upstream SPDX header, verify all import lines are still present afterward. Each vendored file gets the full Apache 2.0 license header plus: + ``` * NOTICE from the Sentry authors: * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree//packages/instrumentation- * - Upstream version: @opentelemetry/instrumentation-@ ``` + Add bullets for TS adjustments or type vendoring only when applicable. Append `/* eslint-disable */` after the header block. Standard replacements in the main instrumentation file: + - Remove `import { PACKAGE_NAME, PACKAGE_VERSION } from './version'` - Add `import { SDK_VERSION } from '@sentry/core'` and `const PACKAGE_NAME = '@sentry/instrumentation-';` - Replace `PACKAGE_VERSION` with `SDK_VERSION` in the `super()` call @@ -111,6 +120,7 @@ yarn build:dev:filter @sentry/ Verify no external types leak into `.d.ts` output. Run existing tests: + - `cd dev-packages/node-integration-tests && yarn test suites/tracing/` - `cd packages/node && yarn test:unit test/integrations/tracing/.test.ts` From 802426f2eedecdeecb40d3197a0c21873014f70b Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 18:00:45 +0200 Subject: [PATCH 4/8] chore: Add confirmation gates and improve PR description format in vendor-otel skill Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index f9c94a3ce6d5..56b3dabfa318 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -47,7 +47,7 @@ Present a plan to the user covering: - Test coverage status - Any concerns -Wait for user approval before implementing. +**Stop here and wait for explicit user approval before implementing.** Use AskUserQuestion to confirm the plan. ## 3. Directory Structure @@ -138,7 +138,9 @@ Before submitting, report ALL modifications to the user: ## 10. PR Creation +**After reporting changes, ask the user if they want to proceed with creating the draft PR.** Use AskUserQuestion to confirm. + - Branch: `vendor--instrumentation` - Commit: `ref(node): Vendor instrumentation` -- PR body: one sentence describing what was vendored, mention inlined types if applicable, reference closing issue +- PR description: one or two concise sentences — what was vendored and any notable details (e.g., inlined types). Reference a closing issue if applicable. Example: "Vendors @opentelemetry/instrumentation-kafkajs into the SDK with no logic changes. Types from kafkajs are inlined as simplified interfaces to avoid requiring the package as a dependency.\n\nCloses #20151" - Always draft PR, base branch `develop` From 7013231f5d6048549fbfac365af75fddcab43bba Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 18:12:49 +0200 Subject: [PATCH 5/8] chore: Always inline external types in vendor-otel skill Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index 56b3dabfa318..673b86032266 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -88,16 +88,16 @@ Include barrel exports (`enums/index.ts`, etc.) if the upstream has them. ## 5. External Type Handling -Types from external packages can leak into `.d.ts` output and break consumers. +**Always inline types from external packages** — even when they only appear in private methods and won't leak into `.d.ts` output. This ensures the SDK compiles standalone without relying on hoisted workspace dependencies. -1. Check if types appear in public class signatures (private-only usage won't leak to `.d.ts`). -2. Check if the upstream `types.ts` already vendors some types inline. -3. If types leak or are needed for compilation, **inline simplified types**: +1. Check if the upstream `types.ts` already vendors some types inline. +2. For any `import type * as X from ''`, **inline simplified types**: - Put in a separate `-types.ts` file in vendored/ - Keep as close to originals as possible — same generic parameters, field names, types + - Only include members actually accessed by the instrumentation - Only simplify when the full type tree is too deep - Add `[key: string]: any` index signatures for permissiveness -4. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` +3. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` ## 6. TypeScript Adjustments From c47dd2e6f008f50b322c58c23c7809b1eef36026 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 18:21:21 +0200 Subject: [PATCH 6/8] chore: Clarify when to inline types vs leave as-is in vendor-otel skill Only inline types from packages that are NOT the instrumented package itself (e.g. @types/*). Types from the instrumented package are always available to consumers. Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index 673b86032266..120d6e027f39 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -88,16 +88,22 @@ Include barrel exports (`enums/index.ts`, etc.) if the upstream has them. ## 5. External Type Handling -**Always inline types from external packages** — even when they only appear in private methods and won't leak into `.d.ts` output. This ensures the SDK compiles standalone without relying on hoisted workspace dependencies. +Types from external packages can leak into `.d.ts` output and break consumers. + +**When to inline vs. leave as-is:** + +- `import type * as X from ''` (e.g., `tedious`, `dataloader`, `generic-pool`) — **do NOT inline**. The instrumented package is always in the user's `node_modules` (otherwise the instrumentation wouldn't run), so its types are guaranteed to be available. +- `import type ... from '@types/'` or types from packages that are NOT the instrumented package itself (e.g., `@types/connect`, `@types/koa`, `@types/amqplib`) — **inline these**. Users may not have the `@types/*` package installed. + +**How to inline:** 1. Check if the upstream `types.ts` already vendors some types inline. -2. For any `import type * as X from ''`, **inline simplified types**: - - Put in a separate `-types.ts` file in vendored/ - - Keep as close to originals as possible — same generic parameters, field names, types - - Only include members actually accessed by the instrumentation - - Only simplify when the full type tree is too deep - - Add `[key: string]: any` index signatures for permissiveness -3. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` +2. Put inlined types in a separate `-types.ts` or `internal-types.ts` file in vendored/ +3. Keep as close to originals as possible — same generic parameters, field names, types +4. Only include members actually accessed by the instrumentation +5. Only simplify when the full type tree is too deep +6. Add `[key: string]: any` index signatures for permissiveness +7. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` ## 6. TypeScript Adjustments From bfd192e8b9d8ed16d76fac0a89733170f9d121da Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 19 May 2026 18:49:37 +0200 Subject: [PATCH 7/8] chore: Always inline external types in vendor-otel skill Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index 120d6e027f39..74984ad0a2cc 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -88,22 +88,16 @@ Include barrel exports (`enums/index.ts`, etc.) if the upstream has them. ## 5. External Type Handling -Types from external packages can leak into `.d.ts` output and break consumers. - -**When to inline vs. leave as-is:** - -- `import type * as X from ''` (e.g., `tedious`, `dataloader`, `generic-pool`) — **do NOT inline**. The instrumented package is always in the user's `node_modules` (otherwise the instrumentation wouldn't run), so its types are guaranteed to be available. -- `import type ... from '@types/'` or types from packages that are NOT the instrumented package itself (e.g., `@types/connect`, `@types/koa`, `@types/amqplib`) — **inline these**. Users may not have the `@types/*` package installed. - -**How to inline:** +**Always inline types from external packages** — including types from the instrumented package itself. Without inlining, the local SDK build relies on workspace hoisting to resolve these types, which is brittle. 1. Check if the upstream `types.ts` already vendors some types inline. -2. Put inlined types in a separate `-types.ts` or `internal-types.ts` file in vendored/ -3. Keep as close to originals as possible — same generic parameters, field names, types -4. Only include members actually accessed by the instrumentation -5. Only simplify when the full type tree is too deep -6. Add `[key: string]: any` index signatures for permissiveness -7. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` +2. For any `import type * as X from ''`, **inline simplified types**: + - Put in a separate `-types.ts` file in vendored/ + - Only include members actually accessed by the instrumentation + - Keep as close to originals as possible — same generic parameters, field names, types + - Only simplify when the full type tree is too deep + - Add `[key: string]: any` index signatures for permissiveness +3. After building, verify no leaks: `grep "from ''" packages/node/build/types/...` ## 6. TypeScript Adjustments From 08800722764bed2e5a33cee6d18316a5407f65a6 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Wed, 20 May 2026 07:57:24 +0200 Subject: [PATCH 8/8] chore: Add changelog review step to vendor-otel skill research phase Co-Authored-By: Claude Opus 4.6 (1M context) --- .agents/skills/vendor-otel/SKILL.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.agents/skills/vendor-otel/SKILL.md b/.agents/skills/vendor-otel/SKILL.md index 74984ad0a2cc..3f9555b32349 100644 --- a/.agents/skills/vendor-otel/SKILL.md +++ b/.agents/skills/vendor-otel/SKILL.md @@ -23,7 +23,15 @@ Check versions: - Latest tag: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags --jq '.[].ref' | grep "instrumentation-"` - Commit SHA: `gh api repos/open-telemetry/opentelemetry-js-contrib/git/refs/tags/instrumentation--v --jq '.object.sha'` -**Diff ALL source files between pinned and latest version.** All OTel instrumentations are pre-v1 so any bump could introduce breaking changes. Report findings to the user. +**Review the upstream CHANGELOG** between pinned and latest version: + +``` +https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/packages/instrumentation-/CHANGELOG.md +``` + +Fetch and present the relevant changelog entries to the user. All OTel instrumentations are pre-v1 so any bump could introduce breaking changes. + +Also **diff ALL source files** between pinned and latest version. Report both the changelog and diff findings to the user so they can verify the bump is safe before proceeding. Check for external type imports (these need special handling, see section 5):