-
Notifications
You must be signed in to change notification settings - Fork 96
Add instrumentation for grpc #343
Add instrumentation for grpc #343
Conversation
Update to main branch
merging all ga effort commits
…encensus-node into add-trace-params
Add trace params
Codecov Report
@@ Coverage Diff @@
## master #343 +/- ##
==========================================
- Coverage 95.17% 95.14% -0.03%
==========================================
Files 122 122
Lines 8472 8472
Branches 750 750
==========================================
- Hits 8063 8061 -2
- Misses 409 411 +2
Continue to review full report at Codecov.
|
packages/opencensus-instrumentation-grpc/src/grpc-stats/client-metrics.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-instrumentation-grpc/src/grpc-stats/client-metrics.ts
Show resolved
Hide resolved
packages/opencensus-instrumentation-grpc/src/grpc-stats/client-metrics.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-instrumentation-grpc/src/grpc-stats/client-metrics.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-instrumentation-grpc/src/grpc-stats/server-metrics.ts
Show resolved
Hide resolved
67108864, 268435456, 1073741824, 4294967296 | ||
]; | ||
export const DEFAULT_MILLI_SECONDS_DISTRIBUTION: number[] = [ | ||
0, 0.01, 0.05, 0.1, 0.3, 0.6, 0.8, 1, 2, 3, 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra space between numbers.
packages/opencensus-instrumentation-grpc/src/grpc-stats/stats-common.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-instrumentation-grpc/src/grpc-stats/stats-common.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, added minor nit comments.
packages/opencensus-instrumentation-grpc/src/grpc-stats/stats-common.ts
Outdated
Show resolved
Hide resolved
@vigneshtdev friendly ping! |
Having to travel too much recently. Will post PR for sure by today |
/** | ||
* {@link View} for server sent messages per RPC. | ||
* | ||
* @since 0.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove since
'Number of response messages received per RPC (always 1 for non-streaming RPCs).'); | ||
|
||
/** | ||
* {@link Measure} for gRPC client uncompressed response bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be " for total bytes received across all response messages per RPC."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added few minor nits
/** | ||
* {@link View} for server received bytes per RPC. | ||
* | ||
* @since 0.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
import {GRPC_BASIC_CLIENT_VIEWS} from './client-metrics'; | ||
import {GRPC_BASIC_SERVER_VIEWS} from './server-metrics'; | ||
|
||
// Common histogram bucket boundaries for bytes received/sets Views. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could you make this a JSDoc comment /** */
and same for DEFAULT_MILLI_SECONDS_DISTRIBUTION
and DEFAULT_MESSAGE_COUNT_DISTRIBUTION
below too?
There are still few measures (like: |
Yeah sure, will add the extra ones in a separate PR. |
Added methods to register the basic gRPC views on both client & server side.Addresses #270