Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add instr-fs and decide on its configuration #240

Closed
trentm opened this issue Jun 13, 2024 · 5 comments · Fixed by #601
Closed

add instr-fs and decide on its configuration #240

trentm opened this issue Jun 13, 2024 · 5 comments · Fixed by #601
Assignees

Comments

@trentm
Copy link
Member

trentm commented Jun 13, 2024

We should add instr-fs as an option at least for the user to enable.

However, it is debatable if this should be enabled by default. Many getting started guides suggest disabling instr-fs or suggest using requireParentSpan: false. (I have not tested the requireParentSpan: false usage to see if that helps deal with the "many fs spans during startup".)

open-telemetry/opentelemetry-js-contrib#1344 is an old (closed by stalebot) discussion around considering disabling instr-fs by default.

@david-luna
Copy link
Member

Upstream has decided to disable it by default in @opentelemetry/auto-instrumenations-node
ref: open-telemetry/opentelemetry-js-contrib#2467

@david-luna
Copy link
Member

Many getting started guides suggest disabling instr-fs or suggest using requireParentSpan: false. (I have not tested the requireParentSpan: false usage to see if that helps deal with the "many fs spans during startup".)

I think you meant to set this value to true.since the default is false. It looks like a good compromise since as a dev I see value on knowing the file system operations that happen in a trace. I'll do a draft PR and we can discuss the details

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

It looks like a good compromise

Now the question is: is it a good enough compromise that EDOT differs from vanilla OTel JS on this.

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

David and I discussed:

  • For GA: let's just follow vanilla OTel JS. The 'fs' use case isn't strong enough to justify subtly differing from vanilla OTel.

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

I think you meant to set this value to true.

Indeed, you are correct. I had it backwards.

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 a pull request may close this issue.

2 participants