Add support for recording gRPC stats #357
Conversation
| import * as clientMetrics from './grpc-stats/client-metrics'; | ||
| import * as serverMetrics from './grpc-stats/server-metrics'; | ||
|
|
||
| const sizeof = require('object-sizeof'); |
There was a problem hiding this comment.
Is this possible to do as a TypeScript import?
There was a problem hiding this comment.
object-sizeof package is missing typings. I tried to use https://github.com/miktam/sizeof/blob/master/index.d.ts but didn;t work.
| status: Status | ||
| status: Status; | ||
| // tslint:disable-next-line:no-any | ||
| request?: any |
There was a problem hiding this comment.
Could this be unknown or the empty object type {} instead?
| /** Method to record stats for client and server. */ | ||
| static recordStats( | ||
| // tslint:disable-next-line:no-any | ||
| kind: SpanKind, tags: TagMap, argsOrValue: any, reqOrRes: any, |
There was a problem hiding this comment.
Could you use the empty object type {} in place of any here? (That should work because you are not actually referencing anything about the type, just transparently passing it into sizeof)
|
|
||
| // record stats | ||
| const tags = new TagMap(); | ||
| tags.set(serverMetrics.GRPC_SERVER_METHOD, {value: rootSpan.name}); |
There was a problem hiding this comment.
As an aside: I think this use case is a good reason to not add Sent. or Recv. to the gRPC span names until they are ready to be exported.
There was a problem hiding this comment.
hmm, makes sense. Looks like need to revise the Java's implementation.
mayurkale22
force-pushed
the
grpc-metrics
branch
from
3c4ff76
to
ec07cc4
Compare
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
- Coverage 95.3% 95.24% -0.06%
==========================================
Files 124 128 +4
Lines 8412 8497 +85
Branches 624 628 +4
==========================================
+ Hits 8017 8093 +76
- Misses 395 404 +9
Continue to review full report at Codecov.
|
mayurkale22
force-pushed
the
grpc-metrics
branch
from
ec07cc4
to
f7e6577
Compare
|
|
||
| const PROTO_PATH = __dirname + '/fixtures/grpc-instrumentation-test.proto'; | ||
| const grpcPort = 50051; | ||
| const MAX_ERROR_STATUS = grpcModule.status.UNAUTHENTICATED; | ||
| const log = logger.logger(); | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: is this extra blank line intentional?
mayurkale22
force-pushed
the
grpc-metrics
branch
from
f7e6577
to
8f9cd70
Compare
This PR is to get the ball rolling, Unit tests are still pending. Reviews are welcome at this moment.
Fixes #270
client-metrics.tsandserver-metrics.tsare updated with minor nits.Update: Tests are included now.