Skip to content

feat: Remove Otel from fsIntegration - #21654

Merged
timfish merged 4 commits into
developfrom
timfish/feat/fs-no-otel
Jun 24, 2026
Merged

feat: Remove Otel from fsIntegration#21654
timfish merged 4 commits into
developfrom
timfish/feat/fs-no-otel

Conversation

@timfish

@timfishtimfish commented Jun 19, 2026•
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR doesn't actually use orchestrion.

Becuase fs in a Node built-in, it is CJS and can therfore be monkey patched like it's 2005.

@timfish
timfish requested a review from a team as a code ownerJune 19, 2026 10:49
@timfish
timfish requested review from JPeer264 and mydea and removed request for a teamJune 19, 2026 10:49
sentry[bot]
sentryBot reviewed Jun 19, 2026
View reviewed changes
Comment threadpackages/node/src/integrations/fs/index.ts Outdated
@mydea

mydea commented Jun 19, 2026

Copy link
Copy Markdown
Member

Becuase fs in a Node built-in, it is CJS and can therfore be monkey patched like it's 2005.

Ah, simpler (better?) times...

@nicohrubec
nicohrubec self-requested a review June 19, 2026 12:01
nicohrubec
nicohrubec reviewed Jun 22, 2026
View reviewed changes
Comment threadpackages/node/src/integrations/fs/index.ts Outdated
recordErrorMessagesAsSpanAttributes: options.recordErrorMessagesAsSpanAttributes,
}),
)();
if (options.enabled === false) return;

@nicohrubecnicohrubecJun 22, 2026•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: Do we need to expose an enabled flag? Afaik we don't have such a flag for other instrumentations and not sure if we need it given that the integration needs to be explicitly added

@timfishtimfishJun 22, 2026•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Ah, because I removed InstrumentationConfig I assumed we needed to retain the enabled to make the change non-breaking but it wasn't exported!

Comment threadpackages/node/src/integrations/fs/index.ts Outdated
nicohrubec
nicohrubec reviewed Jun 22, 2026
View reviewed changes
Comment threadpackages/node/src/integrations/fs/vendored/instrumentation.ts
sentry[bot]
sentryBot reviewed Jun 22, 2026
View reviewed changes
Comment threadpackages/node/src/integrations/fs/vendored/instrumentation.ts
github-advanced-security[bot]
github-advanced-securityBot found potential problems Jun 22, 2026
View reviewed changes
Comment threaddev-packages/node-integration-tests/suites/fs-instrumentation/server.ts Dismissed
@timfish
timfish requested a review from nicohrubecJune 22, 2026 12:46
nicohrubec
nicohrubec approved these changes Jun 22, 2026
View reviewed changes

@nicohrubecnicohrubec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice! lgtm

@timfish
timfish merged commit 7cd5465 into developJun 24, 2026
284 checks passed
@timfish
timfish deleted the timfish/feat/fs-no-otel branch June 24, 2026 08:54
@sentry-release-botsentry-release-botBot mentioned this pull request Jun 25, 2026
Closed
49 tasks
@github-actionsgithub-actionsBot mentioned this pull request Jun 25, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sentrysentry[bot]sentry[bot] left review comments
@nicohrubecnicohrubecnicohrubec approved these changes
@mydeamydeaAwaiting requested review from mydeamydea is a code owner automatically assigned from getsentry/team-javascript-sdks-server
@JPeer264JPeer264Awaiting requested review from JPeer264JPeer264 is a code owner automatically assigned from getsentry/team-javascript-sdks-server
+1 more reviewer
@github-advanced-securitygithub-advanced-security[bot]github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rewrite @opentelemetry/instrumentation-fs to orchestrion

4 participants

@timfish@mydea@nicohrubec@github-advanced-security