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

fix(grpc): patch original client methods #631

Merged
merged 3 commits into from
Dec 20, 2019

Conversation

mayurkale22
Copy link
Member

Which problem is this PR solving?

Short description of the changes

/cc @markwolff

@codecov-io
Copy link

codecov-io commented Dec 17, 2019

Codecov Report

Merging #631 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   91.95%   91.89%   -0.07%     
==========================================
  Files         190      189       -1     
  Lines        9487     9350     -137     
  Branches      856      852       -4     
==========================================
- Hits         8724     8592     -132     
+ Misses        763      758       -5
Impacted Files Coverage Δ
...ckages/opentelemetry-plugin-grpc/test/grpc.test.ts 96.2% <100%> (+0.09%) ⬆️
packages/opentelemetry-plugin-grpc/src/grpc.ts 96.25% <100%> (+0.1%) ⬆️
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts 66.66% <0%> (-8.34%) ⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts 90.9% <0%> (-4.55%) ⬇️
...res/opentelemetry-plugin-pg/test/assertionUtils.ts 96.29% <0%> (-3.71%) ⬇️
...entelemetry-exporter-jaeger/test/transform.test.ts 100% <0%> (ø) ⬆️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/config.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/utility.ts 100% <0%> (ø) ⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts 100% <0%> (ø) ⬆️
... and 14 more

this: typeof grpcTypes.Client,
methods: grpcTypes.ServiceDefinition<ImplementationType>,
methods: { [key: string]: { originalName?: string } },
Copy link
Member

Choose a reason for hiding this comment

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

Is this ServiceDefintion type removed for better readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's right.

shimmer.massWrap(
client.prototype as never,
Object.keys(methods) as never[],
methodsToWrap as never[],
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a never[]

packages/opentelemetry-plugin-grpc/src/grpc.ts Outdated Show resolved Hide resolved
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

Nice! Would be nice to add a test (if not too difficult), perhaps by using the example provided in the issue ?

@OlivierAlbertini OlivierAlbertini added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 20, 2019
@mayurkale22 mayurkale22 merged commit 587a5f5 into open-telemetry:master Dec 20, 2019
@mayurkale22 mayurkale22 deleted the grpc_fix branch December 20, 2019 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC client methods are not patched correctly
5 participants