Add support for recording exemplars #405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
==========================================
+ Coverage 94.82% 94.86% +0.03%
==========================================
Files 136 136
Lines 8935 8990 +55
Branches 656 657 +1
==========================================
+ Hits 8473 8528 +55
Misses 462 462
Continue to review full report at Codecov.
|
| @@ -181,8 +186,13 @@ export interface View { | |||
| * Measurements with measurement type INT64 will have its value truncated. | |||
| * @param measurement The measurement to record | |||
| * @param tags The tags to which the value is applied | |||
| * @param attachments optional The contextual information associated with an | |||
| * example value. THe contextual information is represented as key - value | |||
There was a problem hiding this comment.
Nit typo The not THe
| @@ -114,8 +115,13 @@ export class BaseView implements View { | |||
| * Measurements with measurement type INT64 will have its value truncated. | |||
| * @param measurement The measurement to record | |||
| * @param tags The tags to which the value is applied | |||
| * @param attachments optional The contextual information associated with an | |||
| * example value. THe contextual information is represented as key - value | |||
| timestamp: timestampFromMillis(statsExemplar.timestamp), | ||
| attachments: statsExemplar.attachments | ||
| } | ||
| } as metricBucket; |
There was a problem hiding this comment.
Optional: rather than use a type cast here with a let above, could you make a helper function say getMetricBucket that returns a metricBucket type and then you could even just call buckets.push(getMetricBucket(statsExemplar, bucketCount)) with no intermediate variable?
There was a problem hiding this comment.
Good suggestion, done.
| const buckets = []; | ||
| for (let bucket = 0; bucket < data.bucketCounts.length; bucket++) { | ||
| const bucketCount = data.bucketCounts[bucket]; | ||
| let statsExemplar; |
There was a problem hiding this comment.
Rather than a let, what would you think about using const and a ternary expression, so:
const statsExemplar = exmplars ? exemplars[bucket] : undefined;?
| @@ -176,6 +176,41 @@ describe('Recorder', () => { | |||
| } | |||
| }); | |||
|
|
|||
| describe(`for distribution aggregation data with attachments`, () => { | |||
There was a problem hiding this comment.
Nit: I tend to only use template backticks when something needs to be interpolated
There was a problem hiding this comment.
This is due to copy-paste from previous tests, fixed.
There was a problem hiding this comment.
LGTM overall
Will there be a follow up to automatically add metric exmplars for things like traced HTTP/gRPC calls in those metrics?
Yes, the plan is to modify the plug-ins to record Exemplar by default. Currently we only record SpanContext as Exemplars, so whenever there's a sampled trace it will be recorded in the plug-ins.
Fix typos (THe -> The). Use const and a ternary expression instead of let. Remove unwanted template backticks. Add separate function to create/get metric Bucket (getMetricBucket).
Yes, will create an issue to track this. |
Specs: https://github.com/census-instrumentation/opencensus-specs/blob/18ddbd73ffc6786622fb17a9f1798d6b668fded2/stats/Exemplars.md
Fixes #385