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: merge grpc-js into grpc instrumentation #1657 #1806

Merged
merged 10 commits into from Feb 11, 2021

Conversation

vmarchaud
Copy link
Member

I splitted this PR into 3 commits following my process:

  • i migrated the grpc-js patches into the grpc instrumentation
  • then i refactored the grpc patches to match how grpc-js was architectured
  • finally i updated the main instrumentation class to load both grpc and grpc-js patches.

Fixes #1657

@codecov
Copy link

codecov bot commented Jan 10, 2021

Codecov Report

Merging #1806 (0b9a1b9) into main (1d682c2) will increase coverage by 0.32%.
The diff coverage is 91.28%.

@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
+ Coverage   92.46%   92.78%   +0.32%     
==========================================
  Files         161      182      +21     
  Lines        5229     6363    +1134     
  Branches     1110     1340     +230     
==========================================
+ Hits         4835     5904    +1069     
- Misses        394      459      +65     
Impacted Files Coverage Δ
...es/opentelemetry-instrumentation-grpc/src/utils.ts 93.93% <ø> (ø)
...emetry-instrumentation-grpc/src/instrumentation.ts 76.66% <76.66%> (ø)
...metry-instrumentation-grpc/src/grpc/clientUtils.ts 88.23% <88.23%> (ø)
...elemetry-instrumentation-grpc/src/grpc-js/index.ts 90.81% <90.81%> (ø)
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts 91.54% <91.54%> (ø)
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts 92.50% <92.50%> (ø)
...entelemetry-instrumentation-grpc/src/grpc/index.ts 93.04% <93.04%> (ø)
...metry-instrumentation-grpc/src/grpc/serverUtils.ts 100.00% <100.00%> (ø)
... and 20 more

@vmarchaud vmarchaud changed the title feat: migrate grpc-js into grpc instrumentation #1657 feat: merge grpc-js into grpc instrumentation #1657 Jan 10, 2021
@vmarchaud
Copy link
Member Author

vmarchaud commented Jan 13, 2021

@obecny @dyladan I think i have another way of architecturing this that would avoid exposing protected in the public api, i'm converting it back to draft while i'm working on this

@vmarchaud vmarchaud marked this pull request as draft January 13, 2021 08:28
@vmarchaud vmarchaud force-pushed the grpc-js-instrumentation branch 2 times, most recently from 245e321 to 3950c08 Compare January 16, 2021 14:39
@vmarchaud vmarchaud marked this pull request as ready for review January 16, 2021 14:39
@vmarchaud vmarchaud requested a review from obecny January 16, 2021 14:39
@vmarchaud
Copy link
Member Author

I've rewrote the GrpcInstrumentation class to be some sort of proxy for both GrpcJsInstrumentation and GrpcNativeInstrumentation, it exposes the same API than instrumentation so it should be the same from end users.
PTAL @obecny @dyladan also @markwolff since i believe you wrote initial plugins

@dyladan dyladan added the enhancement New feature or request label Jan 20, 2021
@vmarchaud vmarchaud requested a review from Flarna January 24, 2021 11:41
Base automatically changed from master to main January 25, 2021 19:26
@vmarchaud vmarchaud force-pushed the grpc-js-instrumentation branch 2 times, most recently from a916c67 to 9f93693 Compare January 30, 2021 16:21
@obecny
Copy link
Member

obecny commented Feb 10, 2021

pls remove locks

@vmarchaud
Copy link
Member Author

cc @obecny i've rebased + removed package locks

@vmarchaud vmarchaud merged commit 11d9b19 into open-telemetry:main Feb 11, 2021
@vmarchaud vmarchaud deleted the grpc-js-instrumentation branch February 11, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert grpc-js plugin to instrumentation
5 participants