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

bump @opentelemetry/auto-instrumentations-node to allow enabling only selected instrumentations via env #2745

Conversation

atsu85
Copy link
Contributor

@atsu85 atsu85 commented Mar 11, 2024

Description:

Link to tracking Issue(s):

Testing:

  • Tests are included in the PR of the updated library
    • The change is backwards compatible - all instrumentations are still enabled by default

Documentation:
Usage example: export OTEL_NODE_ENABLED_INSTRUMENTATIONS="http,nestjs-core".
See the documentation for details.

@atsu85 atsu85 requested a review from a team as a code owner March 11, 2024 11:59
@swiatekm swiatekm added Skip Changelog PRs that do not require a CHANGELOG.md entry and removed Skip Changelog PRs that do not require a CHANGELOG.md entry labels Mar 11, 2024
@swiatekm
Copy link
Contributor

swiatekm commented Mar 11, 2024

@atsu85 can you add a changelog entry for this change? Usually we don't do this for dependency upgrades, but this enables a few feature and is worth calling out.

Should we add this as an attribute to the NodeJs instrumentation CR as well? @pavolloffay @jaronoff97 @TylerHelmuth

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

@swiatekm-sumo

can you add a changelog entry for this change?

I thought i already did that by adding this file into .chloggen folder (as documented in Contribution Guidelines: Adding a Changelog Entry). As i understand content from that file will be used to generate an entry into changelog during the releas:

During the collector release process, all ./.chloggen/*.yaml files are transcribed into CHANGELOG.md and then deleted.

Is there anything else i should do?

@pavolloffay
Copy link
Member

Should we add this as an attribute to the NodeJs instrumentation CR as well? @pavolloffay @jaronoff97 @TylerHelmuth

The env var can be already configured in the CR (in a generic way). If the values do not change we could define it as a field in the CR.

@atsu85 do you have a link that lists all instrumentations that can be enabled/disabled?

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

@pavolloffay

do you have a link that lists all instrumentations that can be enabled/disabled?

Sure. I indirectly already referenced to it via

See the documentation for details.

that contains the link you asked for:

By default, all Supported Instrumentations are enabled, but you can use the environment variable OTEL_NODE_ENABLED_INSTRUMENTATIONS to enable only certain instrumentations by providing a comma-separated list of the instrumentation package names without the @opentelemetry/instrumentation- prefix.

@swiatekm
Copy link
Contributor

@swiatekm-sumo

can you add a changelog entry for this change?

I thought i already did that by adding this file into .chloggen folder (as documented in Contribution Guidelines: Adding a Changelog Entry). As i understand content from that file will be used to generate an entry into changelog during the releas:

During the collector release process, all ./.chloggen/*.yaml files are transcribed into CHANGELOG.md and then deleted.

Is there anything else i should do?

Sorry about that, the CI failing confused me. It looks like markdown-link-check is complaining about the anchor in the link you added, though I'm not sure why.

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

It looks like markdown-link-check is complaining about the anchor in the link you added, though I'm not sure why.

It isn't related to the anchor, the message

Check that anchor links are lowercase

is just a hard coded fallback message in GHA pipeline, that isn't correct in this case, as the command fails for different reason.

It seems that there was a bug with the code (thta didn't expect that ignore argument could be missing on that code execution path), and it should be fixed now with this commit. I'm force pushing to re-trigger the build pipeline, because i can't manually re-trigger it

@atsu85 atsu85 force-pushed the 2622-bump-autoinstrumentation-nodejs-to-allow-disabling-instrumentations-via-env branch from c60118d to 0335ca9 Compare March 11, 2024 15:43
to allow enabling only selected instrumentations specified via environment variable `OTEL_NODE_ENABLED_INSTRUMENTATIONS`

open-telemetry#2622
@atsu85 atsu85 force-pushed the 2622-bump-autoinstrumentation-nodejs-to-allow-disabling-instrumentations-via-env branch from 0335ca9 to 80faaf1 Compare March 11, 2024 15:43
@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

@pavolloffay, the GHA changelog workflow is currently waiting for your approval, before it can run again:

This workflow is awaiting approval from a maintainer in #2745

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

@swiatekm-sumo , @pavolloffay, thanks for approving! Now all checks are green.
Can you merge and release it as well, or who can do that?

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@atsu85 would you like to take on the followup PR to the opentelemetry site to update https://opentelemetry.io/docs/kubernetes/operator/automatic/#js-excluding-auto-instrumentation?

@pavolloffay pavolloffay merged commit 95b31fe into open-telemetry:main Mar 11, 2024
32 checks passed
@atsu85 atsu85 deleted the 2622-bump-autoinstrumentation-nodejs-to-allow-disabling-instrumentations-via-env branch March 11, 2024 18:16
@atsu85
Copy link
Contributor Author

atsu85 commented Mar 11, 2024

would you like to take on the followup PR to the opentelemetry site to update https://opentelemetry.io/docs/kubernetes/operator/automatic/#js-excluding-auto-instrumentation?

@TylerHelmuth, sure (created an issue)! But perhaps changes in this repo can be released first, so i could test it first by dogfooding it in our setup?

I see that this part of the documentation (source) is now outdated.

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 12, 2024

would you like to take on the followup PR to the opentelemetry site to update https://opentelemetry.io/docs/kubernetes/operator/automatic/#js-excluding-auto-instrumentation?

@TylerHelmuth, I created a PR to update the documentation - currently in draft status since this PR isn't released, but please review that as well!

@atsu85
Copy link
Contributor Author

atsu85 commented Mar 12, 2024

@pavolloffay, thanks for merging!

@TylerHelmuth, @swiatekm-sumo, @pavolloffay, can you releas the changes, so i could test it first by dogfooding it in our setup, or should i prepare (manually??) the release branch myself based on this?

@pavolloffay
Copy link
Member

@atsu85 the new release can be triggered by updating https://github.com/open-telemetry/opentelemetry-operator/blob/main/autoinstrumentation/nodejs/package.json#L28 along other dependencies.

PRs are welcome :)

atsu85 added a commit to atsu85/opentelemetry-operator that referenced this pull request Mar 12, 2024
To trigger release for open-telemetry#2622 that already bumped auto-instrumentations-node via open-telemetry#2745
@atsu85
Copy link
Contributor Author

atsu85 commented Mar 12, 2024

the new release can be triggered by updating https://github.com/open-telemetry/opentelemetry-operator/blob/main/autoinstrumentation/nodejs/package.json#L28 along other dependencies.

Thanks! I created a PR #2754 - @pavolloffay, please review and approve, so it could start the GHA pipelines

atsu85 added a commit to atsu85/opentelemetry-operator that referenced this pull request Mar 12, 2024
To trigger release for open-telemetry#2622 that already bumped auto-instrumentations-node via open-telemetry#2745
atsu85 added a commit to atsu85/opentelemetry-operator that referenced this pull request Mar 12, 2024
To trigger release for open-telemetry#2622 that already bumped auto-instrumentations-node via open-telemetry#2745
pavolloffay pushed a commit that referenced this pull request Mar 13, 2024
…toinstrumentation/nodejs (#2754)

* Bump all NodeJS devDependences to latest versions

* Bump all NodeJS dependences to latest versions

To trigger release for #2622 that already bumped auto-instrumentations-node via #2745
@atsu85
Copy link
Contributor Author

atsu85 commented Mar 19, 2024

@pavolloffay

the new release can be triggered by updating https://github.com/open-telemetry/opentelemetry-operator/blob/main/autoinstrumentation/nodejs/package.json#L28 along other dependencies.

NB! Looks like this PR actually already overrode release of docker image autoinstrumentation-nodejs:0.46.0, that was previously released with the same version number also for example on 2024.01.18 and 2023.12.20

@pavolloffay, @TylerHelmuth , @swiatekm-sumo, were you aware of it? This seems like a bug to me. Maybe autoinstrumentation-nodejs docker image version shouldn't be read based on @opentelemetry/sdk-node version (that can be left unchanged), but perhaps based on package version in autoinstrumentation/nodejs/package.json that should be bumped with every release?

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
to allow enabling only selected instrumentations specified via environment variable `OTEL_NODE_ENABLED_INSTRUMENTATIONS`

open-telemetry#2622
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…toinstrumentation/nodejs (open-telemetry#2754)

* Bump all NodeJS devDependences to latest versions

* Bump all NodeJS dependences to latest versions

To trigger release for open-telemetry#2622 that already bumped auto-instrumentations-node via open-telemetry#2745
This pull request was closed.
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.

Allow disabling nodejs auto-instrumentations via environment variables
4 participants