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

feat: warn user when a instrumented package was already required #636 #654

Merged
merged 7 commits into from
Feb 10, 2020
Merged

feat: warn user when a instrumented package was already required #636 #654

merged 7 commits into from
Feb 10, 2020

Conversation

vmarchaud
Copy link
Member

No description provided.

@vmarchaud vmarchaud added the enhancement New feature or request label Dec 29, 2019
@vmarchaud vmarchaud self-assigned this Dec 29, 2019
@vmarchaud vmarchaud mentioned this pull request Dec 29, 2019
Closed
@codecov-io
Copy link

codecov-io commented Dec 29, 2019 •
edited
Loading

Codecov Report

Merging #654 into master will decrease coverage by 34.53%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #654       +/-   ##
===========================================
- Coverage   91.11%   56.57%   -34.54%     
===========================================
  Files         232       77      -155     
  Lines       10444     1976     -8468     
  Branches      958      236      -722     
===========================================
- Hits         9516     1118     -8398     
+ Misses        928      858       -70
Impacted Files Coverage Δ
...pentelemetry-core/src/platform/node/performance.ts 0% <0%> (-100%) ⬇️
...try-core/test/trace/fixtures/test-package/index.js 0% <0%> (-100%) ⬇️
...ry-api/test/noop-implementations/noop-span.test.ts 0% <0%> (-100%) ⬇️
...telemetry-core/test/platform/hex-to-base64.test.ts 0% <0%> (-100%) ⬇️
...-api/test/noop-implementations/noop-tracer.test.ts 0% <0%> (-100%) ⬇️
...st/trace/fixtures/test-package/foo/bar/internal.js 0% <0%> (-100%) ⬇️
...ntelemetry-core/src/platform/node/hex-to-base64.ts 0% <0%> (-88.89%) ⬇️
...ckages/opentelemetry-core/test/platform/id.test.ts 0% <0%> (-77.78%) ⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts 0% <0%> (-77.02%) ⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts 0% <0%> (-71.43%) ⬇️
... and 170 more

@naseemkullah
Copy link
Member

As in #636 (comment)

Please confirm: the actual packages (http, express, redis, mysql, etc.) could indeed be required before tracer initialization, but they should be instantiated after the tracer initialization.

@vmarchaud
Copy link
Member Author

vmarchaud commented Jan 1, 2020 •
edited
Loading

I've adressed aboves comments

@vmarchaud vmarchaud force-pushed the warn-on-already-required branch 2 times, most recently from ed609f1 to d0b999b Compare January 5, 2020 12:24
@mayurkale22 mayurkale22 added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. labels Jan 6, 2020
@vmarchaud vmarchaud force-pushed the warn-on-already-required branch 3 times, most recently from 78a933b to 5bb0014 Compare January 11, 2020 16:30
@vmarchaud
Copy link
Member Author

@open-telemetry/javascript-approvers Could we have more review on this one please ?

obecny
obecny approved these changes Jan 20, 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, have one question and I see possible room for performance improvements.

packages/opentelemetry-node/src/instrumentation/PluginLoader.ts Outdated Show resolved Hide resolved
packages/opentelemetry-node/src/instrumentation/PluginLoader.ts Show resolved Hide resolved
dyladan
dyladan previously requested changes Jan 20, 2020
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

#654 (comment)

mayurkale22
mayurkale22 approved these changes Feb 2, 2020
View reviewed changes
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

lgtm, please fix the build for Node 10 and 12.

@mayurkale22 mayurkale22 added The CircleCI build failed and removed needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. labels Feb 2, 2020
@vmarchaud
Copy link
Member Author

i think this one is ready to be merged

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 10, 2020
@mayurkale22 mayurkale22 merged commit b6a2ccd into open-telemetry:master Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obecny obecny obecny approved these changes

@dyladan dyladan dyladan approved these changes

@markwolff markwolff markwolff approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@bg451 bg451 Awaiting requested review from bg451

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

@rochdev rochdev Awaiting requested review from rochdev

Assignees

@vmarchaud vmarchaud

Labels
enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vmarchaud @codecov-io @naseemkullah @obecny @dyladan @markwolff @mayurkale22