Prevent loading plugins for incorrect module #626 #653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vmarchaud
requested review from
bg451,
dyladan,
markwolff,
mayurkale22,
obecny,
OlivierAlbertini and
rochdev
as code owners
Codecov Report
@@ 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
|
dyladan
reviewed
Dec 30, 2019
packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts
Outdated
Show resolved
Hide resolved
vmarchaud
force-pushed
the
verify-package-name
branch
2 times, most recently
from
df15a55
to
a7230b4
Compare
OlivierAlbertini
approved these changes
Jan 2, 2020
vmarchaud
force-pushed
the
verify-package-name
branch
from
a7230b4
to
7dd415c
Compare
|
Tests are failling because |
mayurkale22
added
the
size/M
vmarchaud
force-pushed
the
verify-package-name
branch
from
7dd415c
to
cc9211c
Compare
vmarchaud
added
the
needs-more-reviewers
vmarchaud
force-pushed
the
verify-package-name
branch
2 times, most recently
from
81e3f7c
to
76f575c
Compare
|
@open-telemetry/javascript-approvers Same as the other one, if someone else could review it would be really nice ! |
dyladan
reviewed
Jan 20, 2020
| if (!utils.isSupportedVersion(version, plugin.supportedVersions)) { | ||
| this.logger.error( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
obecny
approved these changes
Jan 27, 2020
packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-node/src/instrumentation/PluginLoader.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-node/src/instrumentation/PluginLoader.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts
Outdated
Show resolved
Hide resolved
dyladan
approved these changes
Jan 28, 2020
vmarchaud
force-pushed
the
verify-package-name
branch
from
7fc728f
to
8d4e560
Compare
|
@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
removed
the
needs-more-reviewers
mayurkale22
approved these changes
Feb 2, 2020
Sorry for the delay, just approved the PR. |
vmarchaud
force-pushed
the
verify-package-name
branch
from
b78c6bb
to
1fd364e
Compare
|
@vmarchaud please resolve the conflicts, looks good to go |
vmarchaud
force-pushed
the
verify-package-name
branch
from
1fd364e
to
8250be1
Compare
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
moduleNameas the name of the module they instrument so i changes theBasePlugininterface andPlugintype.