Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add instrumentation for gRPC on propagating tags and recording gRPC stats #270

Closed
mayurkale22 opened this issue Jan 9, 2019 · 18 comments
Closed

Comments

@mayurkale22
Copy link
Member

Provide instrumentation for gRPC Client and Server to record gRPC stats. Interceptors should also propagate Tags.

@vigneshtdev
Copy link
Contributor

I would like to help out with this issue. Any pointers or comparison with any existing package would be welcome.

@songy23
Copy link
Contributor

songy23 commented Jan 14, 2019

Also see gRPC-Java: CensusStatsModule and CensusTracingModule.

@vigneshtdev
Copy link
Contributor

should i modify opencensus-intrumentation-grpc or create a new package?

@vigneshtdev
Copy link
Contributor

@mayurkale22
Copy link
Member Author

ViewManager in Java is for registering views for collecting stats. In Node, you can do that using Stats API itself. (example: stats.registerView(view)).

@vigneshtdev
Copy link
Contributor

but there is no way to get views in node package

@mayurkale22
Copy link
Member Author

I think we should add gerView(view) inside Stats API. Something like python or Java. As per my understanding, View Data is the aggregated data for a particular view. Currently in Node, we don't have ViewData concept. For now, we can use getSnapshot(tags: Tags): AggregationData; method to get snapshot of an AggregationData. WDYT?

@vigneshtdev
Copy link
Contributor

@mayurkale22
Copy link
Member Author

@vigneshtdev this is good start, I would suggest following changes.

  1. Separate metrics for client and server (client-metrics.ts and server-metrics.ts)
  2. Use new Tags API (Update stats, zpages, prometheus packages with new Tags API #307).
  3. I just see constants in gist, do you plan to add record methods (counterpart in go: https://github.com/census-instrumentation/opencensus-go/blob/e16226179d0f7cb82bbdfed8cd824f39bb53e887/plugin/ocgrpc/stats_common.go)?
  4. Open a PR for code review.

@mayurkale22
Copy link
Member Author

@vigneshtdev I was hoping to see your PR, let me know if you have any question?

@vigneshtdev
Copy link
Contributor

@mayurkale22
Copy link
Member Author

I am not a Go expert, @rghetia Could you please shed light on this?

@rghetia
Copy link

rghetia commented Feb 1, 2019

the above line means if the type of the variable is *stats.OutPayload. For outbound call (either from client or from server) will have stats.OutPayload.

@vigneshtdev
Copy link
Contributor

vigneshtdev commented Feb 3, 2019

grpc-node doesn't have a stats package. So do we assume user shall always pass grpc stats?

In Go's implementation, the statsHandler function accepts grpc stats & further validates them by accessing properties viz;OutPayload InPayload End (https://github.com/census-instrumentation/opencensus-go/blob/2269ed7aa5777d1157c9674a6079207a0f32b68a/plugin/ocgrpc/stats_common.go#L85)

However, java's implementation is even different in that, all the data is plain static. It just registers the views which fall into the predefined set of views. https://github.com/census-instrumentation/opencensus-java/blob/bb02baadd97b753a2740827cffb332665bad1526/examples/src/main/java/io/opencensus/examples/grpc/helloworld/HelloWorldClient.java#L113

http://google.golang.org/grpc/stats

@vigneshtdev
Copy link
Contributor

vigneshtdev commented Feb 12, 2019

@mayurkale22 import { globalStats } from '@opencensus/core'; is not able to find the module globalStats.Any resolve?

And if I use const { globalStats } = require("@opencensus/core"); then I am not able to use it in method signtatue registerAllGrpcViews(stats: globalStats)

This is the ts lint error Parameter 'stats' of public static method from exported class has or is using private name 'globalStats'. [4070]

Fixed by making use Stats from @opencensus/core

@mayurkale22
Copy link
Member Author

@vigneshtdev import { globalStats } from '@opencensus/core'; should work, this is how I have been using everywhere. FYI -> we have release OpenCensus Node (0.0.9) version https://github.com/census-instrumentation/opencensus-node/releases/tag/v0.0.9.

@vigneshtdev
Copy link
Contributor

Hmm strange. I did notice that in the workspace some packages compiled with that import. But some others didn't. Also the version of @opencensus/core was 0.0.8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants