feat: add gRPC Census propagator #39

nkelly75 · 2020-05-28T16:38:08Z

Signed-off-by: Kelly, Niall [email protected]

Which problem is this PR solving?

Aims to address issue #35 i.e. creation of a binary gRPC census propagator. This is intended to provide interoperability with existing services that use gRPC and are instrumented with OpenCensus. I have tested this using a fork of the microservices-demo where most services were already instrumented using OpenCensus but I had modified one node service to use direct OpenTelemetry instrumentation.

Short description of the changes

The new propagator, GrpcCensusPropagator, implements the existing HttpTextPropagator interface allowing its inject/extract implementations to be called from the existing GrpcPlugin in @opentelemetry/plugin-grpc. The binary encoding/decoding actually happens in BinaryTraceContext.ts which is derived from previous work by @mayurkale22 (more details in README.md).

The commit includes unit tests with full coverage of the two main src files.

The new Propagator should compile, lint and test successfully e.g. using lerna bootstrap and lerna run test --scope @opentelemetry/propagator-grpc.

However, I encountered some issues elsewhere in the monorepo e.g.

typescript compiler errors for packages/opentelemetry-test-utils (which impacted unit tests elsewhere)
test failures for `plugins/node/opentelemetry-plugin-mongodb'

I anticipate these may come up as part of the merge checks.

Signed-off-by: Kelly, Niall <[email protected]>

propagators/opentelemetry-propagator-grpc-census-binary/package.json

Signed-off-by: Kelly, Niall <[email protected]>

nkelly75 · 2020-05-28T19:35:42Z

I signed it

mayurkale22

Welcome to OpenTelemetry Community!

Added first pass comments, did you get a chance to verify this propagator in sample example or somewhere else?

mayurkale22 · 2020-05-28T19:57:48Z

propagators/opentelemetry-propagator-grpc-census-binary/package.json

@@ -0,0 +1,69 @@
+{
+  "name": "@opentelemetry/propagator-grpc-census-binary",
+  "version": "0.0.2",


s/0.0.2/0.7.0: this is the current ongoing version of contrib packages (generally, we tried to keep versions in the sync).

mayurkale22 · 2020-05-28T20:00:23Z

propagators/opentelemetry-propagator-grpc-census-binary/package.json

+    "tslint-consistent-codestyle": "^1.16.0",
+    "tslint-microsoft-contrib": "^6.2.0",
+    "typescript": "3.7.2",
+    "webpack": "^4.35.2"


I think this is unused dependency.

mayurkale22 · 2020-05-28T20:01:20Z

propagators/opentelemetry-propagator-grpc-census-binary/package.json

+    "@types/mocha": "^7.0.0",
+    "@types/node": "^12.6.8",
+    "@types/sinon": "^7.0.13",
+    "@types/webpack-env": "1.13.9",


mayurkale22 · 2020-05-28T20:01:32Z

propagators/opentelemetry-propagator-grpc-census-binary/package.json

+  "devDependencies": {
+    "@types/mocha": "^7.0.0",
+    "@types/node": "^12.6.8",
+    "@types/sinon": "^7.0.13",


mayurkale22 · 2020-05-28T20:01:58Z

propagators/opentelemetry-propagator-grpc-census-binary/package.json

+    "codecov": "^3.6.1",
+    "grpc": "^1.24.2",
+    "gts": "^1.1.0",
+    "istanbul-instrumenter-loader": "^3.0.1",


propagators/opentelemetry-propagator-grpc-census-binary/src/BinaryTraceContext.ts

mayurkale22 · 2020-05-28T20:09:07Z

propagators/opentelemetry-propagator-grpc-census-binary/test/index-webpack.ts

+ * limitations under the License.
+ */
+
+// This file is the webpack entry point for the browser Karma tests. It requires


This propagator is not applicable for browser, please remove this file.

Thanks @mayurkale22. I will address your comments tomorrow.

Yes, I did verify this propagator (both inbound and outbound gRPC, from/to services instrumented with OpenCensus) as part of the fork of microservices-demo mentioned in the description.

Latest commit should hopefully cover your first pass @mayurkale22. (FYI, next "I signed it" comment is related to CLA for CNCF).

Signed-off-by: Kelly, Niall <[email protected]>

nkelly75 · 2020-05-29T10:58:43Z

I signed it

WillsonHG · 2020-05-29T13:22:24Z

/check-cla

dyladan · 2020-05-29T13:45:14Z

@WillsonHG I believe @nkelly75 needs to be the one to comment to get the cla rechecked

nkelly75 · 2020-05-29T14:14:07Z

/check-cla

nkelly75 · 2020-05-29T15:32:44Z

I signed it

propagators/opentelemetry-propagator-grpc-census-binary/src/BinaryTraceContext.ts

propagators/opentelemetry-propagator-grpc-census-binary/src/GrpcCensusPropagator.ts

Signed-off-by: Kelly, Niall <[email protected]>

mayurkale22 · 2020-05-30T00:26:32Z

propagators/opentelemetry-propagator-grpc-census-binary/src/GrpcCensusPropagator.ts

+   *
+   * @param context - Context to be injected
+   * @param carrier - Carrier in which to inject (for gRPC this will
+   *                  be a grpc.Metadata object)


I would suggest to add 4 spaces to show the continuation of previous comment.

mayurkale22 · 2020-05-30T00:26:49Z

propagators/opentelemetry-propagator-grpc-census-binary/src/GrpcCensusPropagator.ts

+   * @param carrier - Carrier in which to inject (for gRPC this will
+   *                  be a grpc.Metadata object)
+   * @param setter - setter function that sets the correct key in
+   *                 the carrier


propagators/opentelemetry-propagator-grpc-census-binary/package.json

Signed-off-by: Kelly, Niall <[email protected]>

codecov · 2020-05-31T11:37:52Z

Codecov Report

Merging #39 into master will increase coverage by 0.15%.
The diff coverage is 97.00%.

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   94.32%   94.47%   +0.15%     
==========================================
  Files          57       62       +5     
  Lines        3294     3494     +200     
  Branches      351      376      +25     
==========================================
+ Hits         3107     3301     +194     
- Misses        187      193       +6

Impacted Files	Coverage Δ
...metry-propagator-grpc-census-binary/src/version.ts	`0.00% <0.00%> (ø)`
...pc-census-binary/test/GrpcCensusPropagator.test.ts	`94.50% <94.50%> (ø)`
...gator-grpc-census-binary/src/BinaryTraceContext.ts	`100.00% <100.00%> (ø)`
...tor-grpc-census-binary/src/GrpcCensusPropagator.ts	`100.00% <100.00%> (ø)`
...grpc-census-binary/test/BinaryTraceContext.test.ts	`100.00% <100.00%> (ø)`

Signed-off-by: Kelly, Niall <[email protected]>

dyladan

Looks good to me

Signed-off-by: Kelly, Niall <[email protected]>

dyladan · 2020-06-02T18:22:24Z

@mayurkale22 if you're ok with it, I'll release just this package when it merges as 0.8.0 and make an addendum to the changelog noting that this was released.

mayurkale22 · 2020-06-02T18:37:52Z

@mayurkale22 if you're ok with it, I'll release just this package when it merges as 0.8.0 and make an addendum to the changelog noting that this was released.

Sounds good to me. Let's do that.

feat: add gRPC Census propagator

263edba

Signed-off-by: Kelly, Niall <[email protected]>

nkelly75 requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners May 28, 2020 16:38

chore: better package name

976e429

Signed-off-by: Kelly, Niall <[email protected]>

dyladan reviewed May 28, 2020

View reviewed changes

propagators/opentelemetry-propagator-grpc-census-binary/package.json Outdated Show resolved Hide resolved

chore: package rename omitted from previous commit

aa09e97

Signed-off-by: Kelly, Niall <[email protected]>

mayurkale22 reviewed May 28, 2020

View reviewed changes

chore: remove unused dependencies, update package version and add JSDoc

72f0ce1

Signed-off-by: Kelly, Niall <[email protected]>

dyladan reviewed May 29, 2020

View reviewed changes

propagators/opentelemetry-propagator-grpc-census-binary/src/BinaryTraceContext.ts Show resolved Hide resolved

propagators/opentelemetry-propagator-grpc-census-binary/src/GrpcCensusPropagator.ts Show resolved Hide resolved

chore: add tsdocs on methods

03b0633

Signed-off-by: Kelly, Niall <[email protected]>

mayurkale22 reviewed May 30, 2020

View reviewed changes

Merge branch 'master' into census-propagator

a270418

Signed-off-by: Kelly, Niall <[email protected]>

Kelly, Niall added 2 commits May 31, 2020 12:49

chore: update package and dependencies for 0.8.0

d9a9c9b

Signed-off-by: Kelly, Niall <[email protected]>

style: update indentation for multiline param comments

1e3a9f6

Signed-off-by: Kelly, Niall <[email protected]>

dyladan approved these changes Jun 1, 2020

View reviewed changes

vmarchaud approved these changes Jun 1, 2020

View reviewed changes

chore: update version.ts

3f6a65b

Signed-off-by: Kelly, Niall <[email protected]>

mayurkale22 approved these changes Jun 2, 2020

View reviewed changes

mayurkale22 added the enhancement New feature or request label Jun 2, 2020

mayurkale22 merged commit 193cd1b into open-telemetry:master Jun 2, 2020

nkelly75 deleted the census-propagator branch June 4, 2020 10:07

nkelly75 mentioned this pull request Jun 4, 2020

feat: add gRPC Census Example #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add gRPC Census propagator #39

feat: add gRPC Census propagator #39

nkelly75 commented May 28, 2020

nkelly75 commented May 28, 2020

mayurkale22 left a comment

mayurkale22 May 28, 2020

mayurkale22 May 28, 2020

mayurkale22 May 28, 2020

mayurkale22 May 28, 2020

mayurkale22 May 28, 2020

mayurkale22 May 28, 2020

nkelly75 May 28, 2020

nkelly75 May 29, 2020

nkelly75 commented May 29, 2020

WillsonHG commented May 29, 2020

dyladan commented May 29, 2020

nkelly75 commented May 29, 2020

nkelly75 commented May 29, 2020

mayurkale22 May 30, 2020

mayurkale22 May 30, 2020

codecov bot commented May 31, 2020 •

edited

Loading

dyladan left a comment

dyladan commented Jun 2, 2020

mayurkale22 commented Jun 2, 2020

feat: add gRPC Census propagator #39

feat: add gRPC Census propagator #39

Conversation

nkelly75 commented May 28, 2020

Which problem is this PR solving?

Short description of the changes

nkelly75 commented May 28, 2020

mayurkale22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkelly75 commented May 29, 2020

WillsonHG commented May 29, 2020

dyladan commented May 29, 2020

nkelly75 commented May 29, 2020

nkelly75 commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2020 • edited Loading

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

dyladan commented Jun 2, 2020

mayurkale22 commented Jun 2, 2020

codecov bot commented May 31, 2020 •

edited

Loading