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

@timfish timfish commented Jun 19, 2026

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 owner June 19, 2026 10:49
@timfish timfish requested review from JPeer264 and mydea and removed request for a team June 19, 2026 10:49
Comment thread packages/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
recordErrorMessagesAsSpanAttributes: options.recordErrorMessagesAsSpanAttributes,
}),
)();
if (options.enabled === false) return;

@nicohrubec nicohrubec Jun 22, 2026

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

@timfish timfish Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

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 thread packages/node/src/integrations/fs/index.ts Outdated
Comment thread packages/node/src/integrations/fs/vendored/instrumentation.ts
Comment thread packages/node/src/integrations/fs/vendored/instrumentation.ts
Comment thread dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts Dismissed
@timfish timfish requested a review from nicohrubec June 22, 2026 12:46

@nicohrubec nicohrubec 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 develop Jun 24, 2026
284 checks passed
@timfish timfish deleted the timfish/feat/fs-no-otel branch June 24, 2026 08:54
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.

Rewrite @opentelemetry/instrumentation-fs to orchestrion

4 participants