Stats: Record against implicit (current) tag context #418
Conversation
mayurkale22
requested review from
draffensperger and
justindsmith
as code owners
| // TODO(mayurkale): read tags current execution context | ||
| tags = new TagMap(); | ||
| // Record against implicit (current) context | ||
| tags = getCurrentTagMap(); |
There was a problem hiding this comment.
Will users be able to have fine-grained control over which tags for a given metric come from the context and which should be explicitly specified? Or over which tags in the context are associated with which metric?
There was a problem hiding this comment.
No we intentionally don't provide that level of control. Tags in the context are treated as "opaque" to prevent users accidentally leak confidential information. "Opaque" here means users can insert or upsert tags into context, but they cannot see what tags are in current context or iterate over them.
There was a problem hiding this comment.
OK. Is it possible to have a case where a metric does not have labels associated with all the tags in the context?
There was a problem hiding this comment.
Yes, it is possible. This is the logic to handle that -> https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-core/src/stats/recorder.ts#L47
There was a problem hiding this comment.
Makes sense, thanks
Codecov Report
@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 94.8% 94.89% +0.09%
==========================================
Files 138 140 +2
Lines 9144 9251 +107
Branches 671 672 +1
==========================================
+ Hits 8669 8779 +110
+ Misses 475 472 -3
Continue to review full report at Codecov.
|
| assert.deepEqual(testExporter.recordedMeasurements[0], measurement); | ||
| aggregationData = | ||
| testExporter.registeredViews[0].getSnapshot( | ||
| [{value: 'value1'}, {value: 'value2'}]) as LastValueData; |
There was a problem hiding this comment.
What's the role of the as LastValueData, since it looks like aggregationData already has that type? Can that safely be removed here and below?
There was a problem hiding this comment.
The return type of getSnapshot() method is AggregationData, which is combination of four types SumData|CountData|LastValueData|DistributionData. The conversion (as LastValueData) is added to get exact type of AggregationData.
There was a problem hiding this comment.
Make sense, thanks!
mayurkale22
force-pushed
the
tags_current_context
branch
from
0fef57a
to
7ff24ef
Compare
| break; | ||
| } | ||
| } | ||
| if (existingKey) this.registeredTags.delete(existingKey); |
There was a problem hiding this comment.
Just curious, in Node if I want to update the value for a key in a map, I need to delete the original <key, value> first?
There was a problem hiding this comment.
As per my understanding, this is true if we use object as a key for map. The maps detect existence ( Map.prototype.has ( key )) of keys based on equality (===). Primitives like strings and numbers are compared by their value, while objects (arrays, plain objects) are compared by their reference. That comparison by reference basically checks to see if the objects given refer to the same location in memory.
Basically to check the objects' "value equality" we essentially have to iterate over every property in the objects to see whether they are equal.
A potential option is to use tagkey.name as a key which is a string type. This required lot more changes and would like to handle in separate PR if we really want to do it.
There was a problem hiding this comment.
Make sense, thanks for the explanation. I'd prefer to keep using the TagKey object as the map key.
mayurkale22
force-pushed
the
tags_current_context
branch
from
7ff24ef
to
661e7f1
Compare
Specs are available here: https://github.com/census-instrumentation/opencensus-specs/blob/master/stats/Record.md#recording-stats
This will allow users to set and get the current tag context, which can be used when recoding the stats if explicit tags are not provided.