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.

Sign up for GitHub

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

Jump to bottom

chore: moved plugins-node-all into contrib repo from opentelemetry-js #184

Merged
merged 5 commits into from
Sep 4, 2020
Merged

chore: moved plugins-node-all into contrib repo from opentelemetry-js #184

merged 5 commits into from
Sep 4, 2020

Conversation

michaelgoin
Copy link
Contributor

@michaelgoin michaelgoin commented Aug 27, 2020 •
edited
Loading

Which problem is this PR solving?

  • Migrating plugins-node-all to the contrib repo reduces mismatched otel version issues for plugins due to opentelemetry-js packages being shipped w/o a following contrib release.

See issue: open-telemetry/opentelemetry-js#1370

Short description of the changes

Part 1 of 2? to migrate plugins-node-all into contrib

  • Mostly copy-pasta transfer of package.json and README for plugins-node-all
  • Updated lint workflow to include new "metapackages"
  • Updated lerna to include new "metapackages"
  • Add brief content on metapackage to contrib README

Remaining steps (as I understand):

  1. Ship new versions out of contrib including plugins-node-all
  2. Remove plugins-node-all from opentelemetry-js (step 1 must be completed first)
  3. Profit?!

I'll leave a self-review to highlight some decision points (and potential knowledge gaps) for discussion on my mind.

michaelgoin
michaelgoin commented Aug 27, 2020
View reviewed changes
README.md
@@ -101,3 +107,4 @@ Apache 2.0 - See [LICENSE][license-url] for more information.
[otel-plugin-user-interaction]: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/plugins/web/opentelemetry-plugin-user-interaction
[otel-plugin-xml-http-request]: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/plugins/web/opentelemetry-plugin-xml-http-request
[otel-plugin-express]: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/plugins/node/opentelemetry-plugin-express
[otel-plugins-node-all]: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/metapackages/plugins-node-all
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be broken until merge.

metapackages/plugins-node-all/package.json
@@ -0,0 +1,28 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this in metapackages to be consistent with where/how it was organized in opentelemetry-js. An argument could be made for nesting somewhere in plugins but I do see value in surfacing up it is a package of packages.

I'm open to suggestions if the current approach is not preferred (or we can always tweak later).

metapackages/plugins-node-all/package.json Outdated
Comment on lines 19 to 26
"@opentelemetry/plugin-express": "0.9.0",
"@opentelemetry/plugin-ioredis": "0.9.0",
"@opentelemetry/plugin-mongodb": "0.9.0",
"@opentelemetry/plugin-mysql": "0.9.0",
"@opentelemetry/plugin-pg": "0.9.0",
"@opentelemetry/plugin-pg-pool": "0.9.0",
"@opentelemetry/plugin-redis": "0.9.0",
"@opentelemetry/plugins-node-core": "^0.10.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is my understanding this gets tweaked on release, so I did not touch this area to deal with the current issue. I assume this will get resolved when we release/publish. Let me know if that is not the right approach.

metapackages/plugins-node-all/README.md
@@ -0,0 +1,53 @@
# OpenTelemetry Plugins Node All
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the naming here. It was claiming to also be "Node Core"

.github/workflows/lint.yml
@@ -16,6 +16,7 @@ jobs:
path: |
node_modules
packages/*/node_modules
metapackages/*/node_modules
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like the only place that likely needing something added. Nothing in the CircleCI setup seemed like it would need tweaking but perhaps I missed something.

Copy link
Member

Choose a reason for hiding this comment

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

You also need to add metapackages to the lerna.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant with regards to the CI configuration files (circle VS actions). I did add to lerna.json with this PR but definitely double-check I did correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I just saw "This seemed like the only place that likely needing something added" and my brain was on autopilot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I definitely should have been clearer.

@michaelgoin michaelgoin marked this pull request as ready for review August 27, 2020 22:17
@michaelgoin michaelgoin requested a review from a team August 27, 2020 22:17
@codecov
Copy link

codecov bot commented Aug 31, 2020 •
edited
Loading

Codecov Report

Merging #184 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #184   +/-   ##
=======================================
  Coverage   95.22%   95.22%           
=======================================
  Files          93       93           
  Lines        4756     4756           
  Branches      492      492           
=======================================
  Hits         4529     4529           
  Misses        227      227

obecny
obecny approved these changes Sep 1, 2020
View reviewed changes
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

vmarchaud
vmarchaud approved these changes Sep 1, 2020
View reviewed changes
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.

lgtm, minor comment with the new release on the main repo

metapackages/plugins-node-all/package.json Outdated
"@opentelemetry/plugin-pg": "0.9.0",
"@opentelemetry/plugin-pg-pool": "0.9.0",
"@opentelemetry/plugin-redis": "0.9.0",
"@opentelemetry/plugins-node-core": "^0.10.2"
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 we can bump this to 0.11.0

Copy link
Member

Choose a reason for hiding this comment

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

@michaelgoin can you resolve this then we can merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@michaelgoin
Copy link
Contributor Author

Per conversation in SIG meeting today, will be renaming this via the new "instrumentation" naming standard which will also allow us to match versions, etc. to avoid the ongoing version issues with the old plugin.

@dyladan
Copy link
Member

dyladan commented Sep 2, 2020 •
edited
Loading

@michaelgoin up to you if you want to do instrumentation rename in this PR or hold it for a follow-up PR. It might be easier to get it reviewed/merged as 2 separate? Since this is already approved, I can just merge it and you can work on the rename in a new PR?

Let me know. Whichever you decide is fine.

@michaelgoin
Copy link
Contributor Author

@dyladan sure. lets get this merged and i'll get a new PR in for the rename.

@michaelgoin
Copy link
Contributor Author

@dyladan think this is good to go now

@dyladan dyladan merged commit 1702d94 into open-telemetry:master Sep 4, 2020
@obecny obecny added the enhancement New feature or request label Sep 7, 2020
@michaelgoin michaelgoin deleted the migrate-plugins-node-all branch September 10, 2020 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@obecny obecny obecny approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelgoin @dyladan @obecny @vmarchaud