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

Prevent loading plugins for incorrect module #626 #653

Merged
merged 2 commits into from
Feb 12, 2020

Conversation

vmarchaud
Copy link
Member

we needed to avoid loading a plugin if the module name in the config was different than the one that the plugin instrument.
It requires that every package expose moduleName as the name of the module they instrument so i changes the BasePlugin interface and Plugin type.

@codecov-io
Copy link

codecov-io commented Dec 29, 2019

Codecov Report

Merging #653 into master will increase coverage by 0.03%.
The diff coverage is 90.27%.

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
+ Coverage   92.62%   92.65%   +0.03%     
==========================================
  Files         221      228       +7     
  Lines       10241    10312      +71     
  Branches      925      943      +18     
==========================================
+ Hits         9486     9555      +69     
- Misses        755      757       +2
Impacted Files Coverage Δ
packages/opentelemetry-plugin-dns/src/version.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-plugin-redis/src/version.ts 100% <ø> (ø) ⬆️
...s/opentelemetry-tracing/src/BasicTracerRegistry.ts 100% <ø> (ø) ⬆️
...lemetry-core/test/trace/globaltracer-utils.test.ts 96.96% <ø> (ø) ⬆️
...ckages/opentelemetry-plugin-mongodb/src/version.ts 100% <ø> (ø) ⬆️
...stgres/opentelemetry-plugin-pg-pool/src/version.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-plugin-http/src/version.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-plugin-https/src/version.ts 0% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/version.ts 0% <0%> (ø) ⬆️
...kages/opentelemetry-exporter-jaeger/src/version.ts 0% <0%> (ø) ⬆️
... and 65 more

@vmarchaud vmarchaud self-assigned this Jan 1, 2020
@vmarchaud vmarchaud force-pushed the verify-package-name branch 2 times, most recently from df15a55 to a7230b4 Compare January 1, 2020 17:04
@vmarchaud
Copy link
Member Author

Tests are failling because https://david-dm.org which is used for badges is apparently down, i believe it's safe to merge as-is and maybe fix that in another PR

@mayurkale22 mayurkale22 added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 6, 2020
@vmarchaud vmarchaud added the needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. label Jan 11, 2020
@vmarchaud vmarchaud force-pushed the verify-package-name branch 2 times, most recently from 81e3f7c to 76f575c Compare January 11, 2020 16:25
@vmarchaud
Copy link
Member Author

vmarchaud commented Jan 19, 2020

@open-telemetry/javascript-approvers Same as the other one, if someone else could review it would be really nice !

if (!utils.isSupportedVersion(version, plugin.supportedVersions)) {
this.logger.error(
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be a warning not an error. An error implies something is broken, where a warning is just telling the user they may be doing something they should not.

Copy link
Member Author

@vmarchaud vmarchaud Jan 20, 2020

Choose a reason for hiding this comment

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

I would like some more input on that, i personnaly prefer to raise an error since the plugin wont work.

cc @open-telemetry/javascript-approvers

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, left a comments with regards to copy only

@dyladan dyladan added the enhancement New feature or request label Jan 28, 2020
@vmarchaud
Copy link
Member Author

@mayurkale22 If you have no objection on the error level above, i believe it's good to merge (tests are failing on 8 for one of our dependency like usual and the lint because api isnt published yet)

@mayurkale22 mayurkale22 removed the needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. label Feb 2, 2020
@mayurkale22
Copy link
Member

@mayurkale22 If you have no objection on the error level above, i believe it's good to merge (tests are failing on 8 for one of our dependency like usual and the lint because api isnt published yet)

Sorry for the delay, just approved the PR.

@mayurkale22
Copy link
Member

@vmarchaud please resolve the conflicts, looks good to go

@dyladan dyladan merged commit b1fe2c8 into open-telemetry:master Feb 12, 2020
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…-telemetry#653)

* fix: do not load plugin when they patch a different module than defined in config open-telemetry#626
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
open-telemetry#653)

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants