Add tags package based on the specs. #172
Conversation
bogdandrutu
requested review from
mayurkale22,
rghetia,
SergeyKanzhelev and
songy23
| // each key is associated with exactly one value. | ||
| message TagMap { | ||
| // Not a map becuase the proto3 map does not allow a message as the key. | ||
| repeated Tag tags = 1; |
There was a problem hiding this comment.
so tags with the same name is allowed, correct? Perhaps something should be said about it in comment
There was a problem hiding this comment.
The list should contains unique TagKeys. The proto3 map does not allow the map_key to be a proto message see https://developers.google.com/protocol-buffers/docs/proto3#maps.
There was a problem hiding this comment.
The fact that the same key value is not allowed is not clear from the comment at all. As minimum I suggest to indicate this requirement in a comment.
There was a problem hiding this comment.
From my perspective - compromising on data integrity by not forcing uniqueness of keys in favor of potential extensibility sounds a hard sell in this case. Do you see a scenario in which key will be typed? Any other information like PII flag may be moved to the value. Or I am missing something?
There was a problem hiding this comment.
BTW, map with the string key is used for Attributes already. Arguably Attributes keys should have all the same scenarios as tags.
There was a problem hiding this comment.
Just to clarify - some misunderstanding may be coming from how you can read the sentence "each key is associated with exactly one value.". If you treat "each key" as unique string - it is already handled. But I read it as one key object maps to one value object. I see you already handled it, just clarifying what I meant,
There was a problem hiding this comment.
The reason of using a TagKey instead of directly string is because few reasons:
- Later we can make the TagKey { oneof (string, int) } where int may represent a global id if you have a global registry for all the tags.
- For the string in TagKey we need to do validation (limited char set) so this way people can create the key once and avoid validation every time in the api.
| // It MUST contain only printable ASCII (codes between 32 and 126) | ||
| // | ||
| // This field is required. | ||
| string value = 1; |
There was a problem hiding this comment.
why not truncatable string? If it is not a truncatable string - what's the reason to have a message for it and not simply use string as before? Same comment for TagKey
There was a problem hiding this comment.
The reason to have a message:
- For the tag key in the future we may add other things - e.g. type information (string, int, etc.) or any other metadata.
- For the tag value in the future we may support other types like int.
There was a problem hiding this comment.
I got why we need a message for TagValue. This comment is more about the fact that there is no consistency in current proto definitions on whether string or TruncatableString is used. Or I am missing something. AttributeValue and Span name uses TruncatableString whereas Status and Tracestate uses string. So my question is what's the deciding factor here of whether to use one or another
| // * MUST not be empty. | ||
| // | ||
| // This field is required. | ||
| string value = 1; |
There was a problem hiding this comment.
I think this should be like -> string name = 1;
| // TagValue is the value associated with a Tag. | ||
| message TagValue { | ||
| // It MUST contain only printable ASCII (codes between 32 and 126) | ||
| // |
There was a problem hiding this comment.
It cannot be longer than 256 right? https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/tags/TagValue.java#L49
Based on the specs:
https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md