-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[operator/nodejs] document disabling node auto-instrumentations #4824
[operator/nodejs] document disabling node auto-instrumentations #4824
Conversation
Wait don't merge this I have a typo on a link |
Fixed! Side note: I noticed in a separate issue that there is external link checking but it is currently excluding GitHub URLs, which is how this one got missed. Noting for anyone curious (I was 😄 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing those links! I noticed the broken http link as well. (@chalin not sure if this needs attention or if we're happy to continue ignoring GH links.)
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. See inline suggestion.
Also, what happens if both OTEL_NODE_ENABLED_INSTRUMENTATIONS
and OTEL_NODE_DISABLED_INSTRUMENTATIONS
are provided? Should there be a note explaining this?
Yes, thanks for fixing the link. I don't have the time to enable the link checking for GH links atm. If it becomes problematic (i.e., we feel that we've been manually detecting too many broken links into GH repos), then we can bump the priority on enabling such link checks. |
All instrumentations are disabled because they are mutually exclusive. I can add that note. I've opened a PR on js-contrib to illustrate this behavior and if there is feedback to change behavior or update the debug message I can come back and open a new PR for that. |
Co-authored-by: Patrice Chalin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to avoid future tense if possible, so there's no question of when the thing happens. Otherwise, LGTM! Thanks.
@chalin I added the note for behavior of both env vars being set, thanks for calling it out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that @trentm's comments are blocking I put this in to not accidentally merge it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and I'm glad for this to get merged as is for now, but:
- We might prefer the use of an
{{% alert %}}
short code instead of blockquote markdown - @svrnm - do we want to pull out that note into a shortcode given that it's used verbatim in two place. This would avoid drift.
Will let @svrnm have the final call here before this get's merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem like there is some precedent in the content to use > ...
blockquote markdown syntax for side-notes / call-outs.
Agreed, but let's leave it like that to get this PR merged and we can fix this eventually.
Yes, but we try to replace them whenever we handle an existing file, eventually we want to use the shortcode everywhere since it gives us more flexibility in styling & structuring those call outs. I will fix the notice and merge this PR |
/fix:all |
You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/10073657559 |
thank you all! |
Ah, good to know. I had that and removed it since it seemed like it was no longer a note that needed major attention. I thought it might be a "downgraded" note 😅 |
Summary
OTel JS auto-instrumentations-node v0.48.0 now includes the ability to disable instrumentations using environment variables.
Changes
I'm not sure if this is too redundant in this text, as it's mostly a copy-paste. Open to suggestions!