Skip to content

feat(otel-node): add fs instrumentation#601

Merged
david-luna merged 8 commits into
mainfrom
240-instr-fs
Feb 12, 2025
Merged

feat(otel-node): add fs instrumentation#601
david-luna merged 8 commits into
mainfrom
240-instr-fs

Conversation

@david-luna
Copy link
Copy Markdown
Member

This PR adds @opentelenetry/instrumentation-fs and follows the consensus of the OTEL JS SIG on having it disabled by default.
ref: open-telemetry/opentelemetry-js-contrib#2453
ref: #240 (comment)

The included tests check for enabling it via environment variable but not via passing a config object in getInstrumentations method.

@trentm let's discuss if test is needed and also where to put in the docs how to enable the instrumentation.

Closes: #240

@david-luna david-luna requested a review from trentm February 10, 2025 15:52
Copy link
Copy Markdown
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

LGTM with some nits.

| `@opentelemetry/instrumentation-cassandra-driver` | `cassandra-driver` version range `>=4.4.0 <5` | [README](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-cassandra#readme) |
| `@opentelemetry/instrumentation-express` | `express` version range `^4.0.0` | [README](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-express#readme) |
| `@opentelemetry/instrumentation-fastify` | `fastify` version range `>=3 <5` | [README](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-fastify#readme) |
| `@opentelemetry/instrumentation-fs` | `fs` module for Node.js `>=14` | [README](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/instrumentation-fs#readme) |
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.

for Node.js >=14

I wonder if we should drop the "for Node.js [VERSION]" specifier now. We've bumped our min-supported Node.js now. Rather than having that version range in a multiple places, it can just be once in the "Node.js versions" section. Anyway, this can be done separately.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could rewrite it to

`fs` module for suppported Node.js versions

and apply the same for other node:* instrumentations like net

Comment thread packages/opentelemetry-node/test/instr-fs.test.js Outdated
Co-authored-by: Trent Mick <trent.mick@elastic.co>
@david-luna
Copy link
Copy Markdown
Member Author

I'll merge after #603 is complete. This instrumentation can go into next version

@david-luna david-luna merged commit acdcab5 into main Feb 12, 2025
@david-luna david-luna deleted the 240-instr-fs branch February 12, 2025 16:11
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.

add instr-fs and decide on its configuration

2 participants