-
Notifications
You must be signed in to change notification settings - Fork 97
Update stats, zpages, prometheus packages with new Tags API #307
Update stats, zpages, prometheus packages with new Tags API #307
Conversation
TagValue[] { | ||
const tagValues: TagValue[] = []; | ||
|
||
// ignore not found key values. |
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.
For tag keys that are not found, we should use null
or undefined
as the tag value. See https://github.com/census-instrumentation/opencensus-java/blob/5a97afdefdb2d9f4213264d91fa67b646adbf5d9/impl_core/src/main/java/io/opencensus/implcore/stats/RecordUtils.java#L86-L87.
The tag values returned from this method should have the same size as tag keys.
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 project has strictNullChecks
set to false in tsconfig, hence didn't add null for missing tag values.
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.
The caveat here is the tag values returned must match the order of tag keys. If we skip any tag values then they became mismatched.
One example: a view is defined to break down latencies by method and caller. The tag keys are [method, caller]
in order. If a record
is called with only tag caller:c1
, this method will return tag values [c1]
. Then c1
will be matched with method
.
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.
Make sense, For tag keys that are not found, replaced with null
as the tag value.
packages/opencensus-exporter-prometheus/src/prometheus-stats.ts
Outdated
Show resolved
Hide resolved
@@ -175,5 +175,49 @@ describe('Recorder', () => { | |||
}); | |||
} | |||
}); | |||
|
|||
describe('getTagValues()', () => { |
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.
There should be another test case on missing and mis-ordered tag values to columns.
TagValue[] { | ||
const tagValues: TagValue[] = []; | ||
|
||
// ignore not found key values. |
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.
The caveat here is the tag values returned must match the order of tag keys. If we skip any tag values then they became mismatched.
One example: a view is defined to break down latencies by method and caller. The tag keys are [method, caller]
in order. If a record
is called with only tag caller:c1
, this method will return tag values [c1]
. Then c1
will be matched with method
.
Fixes #305
This PR contains breaking API changes, highlighted in the CHANGELOG with old and new code.