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

feat(plugin): add supportedVersions property #237

Merged

Conversation

@OlivierAlbertini
Copy link
Member

commented Sep 5, 2019

Which problem is this PR solving?

It's backward compatible. supportedVersion is optional in plugins. If not defined, all versions are supported. See related tests.

Closes #132

Signed-off-by: Olivier Albertini olivier.albertini@montreal.ca

@codecov-io

This comment has been minimized.

Copy link

commented Sep 5, 2019

Codecov Report

Merging #237 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   98.79%   98.77%   -0.03%     
==========================================
  Files          55       55              
  Lines        2161     2118      -43     
  Branches      151      149       -2     
==========================================
- Hits         2135     2092      -43     
  Misses         26       26
Impacted Files Coverage Δ
...metry-core/src/trace/instrumentation/BasePlugin.ts 12.5% <ø> (ø) ⬆️
...try-node-tracer/test/instrumentation/utils.test.ts 100% <100%> (ø) ⬆️
...ry-node-tracer/src/instrumentation/PluginLoader.ts 90% <100%> (+0.41%) ⬆️
...e-tracer/test/instrumentation/PluginLoader.test.ts 100% <100%> (ø) ⬆️
...telemetry-node-tracer/src/instrumentation/utils.ts 96.55% <100%> (+0.71%) ⬆️
...kages/opentelemetry-basic-tracer/test/Span.test.ts 100% <0%> (ø) ⬆️
...sic-tracer/test/export/SimpleSpanProcessor.test.ts 100% <0%> (ø) ⬆️
@mayurkale22
Copy link
Contributor

left a comment

Nice! How do you set the supportedVersions? Is it going to be part of PluginConfig (pull/229)?

It would be nice If you can add 1-2 tests in PluginLoader.test.ts.

@OlivierAlbertini

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Nice! How do you set the supportedVersions? Is it going to be part of PluginConfig

When developer write a plugin, it need to specify all supported versions, see Plugin interface and BasicPlugin abstract class.

When the hook is triggered, we check if the version is satisfied.

if (!utils.isSupportedVersion(version, plugin.supportedVersions)) {
     return exports;
}
@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Ok, got it, thanks for the clarification. 👍

@mayurkale22
Copy link
Contributor

left a comment

Otherthan few minors, LGTM.

@markwolff
Copy link
Member

left a comment

LGTM. To verify, empty array [] means that no versions are supported and so no patching would be done?

feat(plugin): add supportedVersions property
Closes #132

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

@OlivierAlbertini OlivierAlbertini force-pushed the VilledeMontreal:feature/supportedversions branch from 81b0d0d to d1f8dcd Sep 5, 2019

@OlivierAlbertini

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

LGTM. To verify, empty array [] means that no versions are supported and so no patching would be done?

Yeah sounds not safe... So I added a test and now it's the same behaviour than undefined

@OlivierAlbertini

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

It would be nice If you can add 1-2 tests in PluginLoader.test.ts.

Done.

@OlivierAlbertini OlivierAlbertini requested a review from vmarchaud Sep 5, 2019

@markwolff

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

LGTM. To verify, empty array [] means that no versions are supported and so no patching would be done?

Yeah sounds not safe... So I added a test and now it's the same behaviour than undefined

Actually, I think it makes sense for it not to do patching in this case since it'd be explicitly set to "no versions". On the other hand, I'm not sure why a non-patching plugin would exist in first place. What do you think?

fix: add recommendations
from markwolff
from mayurkale22

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

@OlivierAlbertini OlivierAlbertini force-pushed the VilledeMontreal:feature/supportedversions branch from d1f8dcd to 8547b6d Sep 5, 2019

@OlivierAlbertini

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Is it going to be part of PluginConfig (pull/229)?

It's interesting! Let's say, developer sets some supportedVersions... and client find a version that is not supported but works for some reasons. It could override this without waiting a PR approval etc.. ?

We could create a separate issue for that if it's needed. Let me know.

@mayurkale22
Copy link
Contributor

left a comment

Thanks for the addressing the comments and adding more tests.

@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

It's interesting! Let's say, developer sets some supportedVersions... and client find a version that is not supported but works for some reasons. It could override this without waiting a PR approval etc.. ?

Agreed. I like the current approach.

@vmarchaud
Copy link
Member

left a comment

LGTM but i would rename the node_modules folder in test since it can be confusing, the require could just use the full path, but it's non-blocking for me

@OlivierAlbertini

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

LGTM. To verify, empty array [] means that no versions are supported and so no patching would be done?

Yeah sounds not safe... So I added a test and now it's the same behaviour than undefined

Actually, I think it makes sense for it not to do patching in this case since it'd be explicitly set to "no versions". On the other hand, I'm not sure why a non-patching plugin would exist in first place. What do you think?

In order to reduce possible frustrations,[] and undefined should have the same behaviour. If you don't specify at least a version, it means that all versions is supported. As you said, it makes no sense to set [] in order to deactivate the plugin (we can do that with PluginConfig)

@mayurkale22 mayurkale22 merged commit 77a25dc into open-telemetry:master Sep 7, 2019

8 checks passed

ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node11 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation mayurkale22 authorized
Details

@OlivierAlbertini OlivierAlbertini deleted the VilledeMontreal:feature/supportedversions branch Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.