This repository has been archived by the owner on Oct 3, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add tags package based on the specs. #172
base: master
Are you sure you want to change the base?
Add tags package based on the specs. #172
Changes from 1 commit
85fd464
5d2823d
879ea3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think this should be like ->
string name = 1;
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.
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
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.
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 forTagKey
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 reason to have a message:
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.
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 whetherstring
orTruncatableString
is used. Or I am missing something. AttributeValue and Span name usesTruncatableString
whereasStatus
andTracestate
usesstring
. So my question is what's the deciding factor here of whether to use one or anotherThere 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW,
map
with thestring
key is used forAttributes
already. ArguablyAttributes
keys should have all the same scenarios as tags.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason of using a TagKey instead of directly string is because few reasons: