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

remove plugins #2081

Merged
merged 13 commits into from
Apr 9, 2021
Merged

remove plugins #2081

merged 13 commits into from
Apr 9, 2021

Conversation

obecny
Copy link
Member

@obecny obecny commented Apr 7, 2021

Which problem is this PR solving?

Short description of the changes

  • removing plugin loader
  • removing BasePlugin
  • removing meta packages
  • removing old plugins (http + grpc)
  • updating readme & examples

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #2081 (7a3ac72) into main (4a468fc) will decrease coverage by 0.52%.
The diff coverage is 100.00%.

❗ Current head 7a3ac72 differs from pull request most recent head ac77f5b. Consider uploading reports for the commit ac77f5b to get more accurate results

@@            Coverage Diff             @@
##             main    #2081      +/-   ##
==========================================
- Coverage   93.08%   92.55%   -0.53%     
==========================================
  Files         154      133      -21     
  Lines        5999     4848    -1151     
  Branches     1258     1000     -258     
==========================================
- Hits         5584     4487    -1097     
+ Misses        415      361      -54     
Impacted Files Coverage Δ
...kages/opentelemetry-node/src/NodeTracerProvider.ts 100.00% <ø> (ø)
packages/opentelemetry-sdk-node/src/sdk.ts 78.12% <ø> (+1.65%) ⬆️
...ackages/opentelemetry-web/src/WebTracerProvider.ts 100.00% <ø> (ø)
...es/opentelemetry-instrumentation/src/autoLoader.ts 100.00% <100.00%> (ø)
...entelemetry-instrumentation/src/autoLoaderUtils.ts 91.66% <100.00%> (-8.34%) ⬇️
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

README.md Outdated Show resolved Hide resolved
doc/instrumentation-guide.md Show resolved Hide resolved
@@ -162,7 +162,7 @@ myPLugin.enable();
Successor of loading plugins through TracerProvider "plugins" option.
It also supersedes PluginLoader for node. The old configurations usually looks like

### NODE - old way using TracerProvider
### NODE - old way using TracerProvider - not available anymore
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this is it is no longer available?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had hard times to decide whether to remove it or it should remain for some time so when people will be upgrading they can at least compare what has changed. That was the reason I left it but really not sure what is better from user perspective :/, can remove it as well

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to keep it for a while. But I think we should change it to something like removed since 0.19.0.

packages/opentelemetry-node/README.md Outdated Show resolved Hide resolved
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

Feel really good to finally remove all that code after lot of efforts 🚀

"@opentelemetry/plugin-express": "^0.14.0",
"@opentelemetry/plugin-http": "^0.18.2",
"@opentelemetry/plugin-https": "^0.18.2",
"@opentelemetry/instrumentation-express": "^0.15.0",
Copy link
Member

Choose a reason for hiding this comment

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

Not needed in this PR - but I think we should get rid of dependencies to contrib in this repo.

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.

Remove PluginLoader from instrumentation Remove BasePlugin
4 participants