Add initial spec for DistributedContext.Entry #73
Conversation
songy23
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
SergeyKanzhelev,
tedsuo and
yurishkuro
as code owners
|
Is there room to debate the term |
|
@jmacd happy to hear options :) I am personally also not convinced about the name. |
|
As part of this PR., can you please remove copied parts from https:/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/specification/tags/TagMap.md So we know what's left to cover from original specs in a new specs |
|
For #46 - please copy pieces of terminology into this document: https:/open-telemetry/opentelemetry-specification/blob/master/terminology.md#tags Terminology document explains the terms and used and how their purpose |
| @@ -0,0 +1,95 @@ | |||
| # Summary | |||
There was a problem hiding this comment.
if this is an API document - it lacks description of supported operations and types. Please add
There was a problem hiding this comment.
Just looked at @jmacd's PR. I believe it can be merged from there.
There was a problem hiding this comment.
Has there been a discussion about keeping or dropping the OC concept of mutators, e.g., being able to delete a key from a context?
|
Is there a record of the past discussion, where people expressed confusion over |
|
@jmacd so far the reason I know is that OpenTracing had tags before as a different thing. Also some people had a concern that if we call tags users cannot use this for other purposes than "telemetry" - which arguably should not, but I can see reasons to not have 2 different distributed contexts. |
Discussions can be found at open-telemetry/opentelemetry-java#11. That issue has more detailed background too.
|
|
For the record, I really liked |
|
Please, let's create another issue for this and will move on with the names we chose. Let's not block PR on the past decision, which @songy23 wasn't even part of. (I realize it might have been a wrong decision. Like many others we did.) |
|
|
||
| ## EntryValue | ||
|
|
||
| `EntryValue` is a string. It MUST contain only printable ASCII (codes between |
There was a problem hiding this comment.
Where does the character-set restriction come from?
There was a problem hiding this comment.
These are inherited from the OC specs: https:/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/specification/tags/TagMap.md.
songy23
force-pushed
the
distributedcontext
branch
from
17fdd45
to
e58f318
Compare
songy23
force-pushed
the
distributedcontext
branch
from
e58f318
to
66f4adc
Compare
|
@SergeyKanzhelev Thanks for the feedback - all addressed. For now I kept the name as open-telemetry/opentelemetry-java#11. Will wait on #75. |
songy23
force-pushed
the
distributedcontext
branch
from
66f4adc
to
fb49b5f
Compare
songy23
force-pushed
the
distributedcontext
branch
from
fb49b5f
to
0260d9f
Compare
|
Based on our conversation today we'll stick with the new names for now. Any other comments expect the naming? |
| - **EntryValue** is a string that contains only printable ASCII (codes between 32 and 126). | ||
| - **EntryMetadata** contains properties associated with an **Entry**. | ||
| For now only the property **EntryTTL** is defined. | ||
| - **EntryTTL** is an integer that represents number of hops an entry can propagate. |
There was a problem hiding this comment.
What's the maximum value of EntryTTL (e.g. 2^16 - 2)?
There was a problem hiding this comment.
We don't have a limit on EntryTTL. Since we've defined a specific Unlimited (+Inf) value, IMO we don't need an explicit maximum here.
There was a problem hiding this comment.
In languages which support BigInt (e.g. Python by default uses bigint), we expect users to pass in a huge number (e.g. 12345678901234567890) and we should handle it without giving error?
There was a problem hiding this comment.
I think this is more a language-specific issue since integer in different languages can have different range. Filed #93 to see if it's worth adding an explicit maximum.
| - **EntryMetadata** contains properties associated with an **Entry**. | ||
| For now only the property **EntryTTL** is defined. | ||
| - **EntryTTL** is an integer that represents number of hops an entry can propagate. | ||
| Anytime a sender serializes an entry, sends it over the wire and receiver unserializes |
There was a problem hiding this comment.
One clarification question, if the receiver simply bypass the entries (e.g. proxy), it is not considered one hop?
We might need to clarify what happens if there are multiple entries, do we allow some of them to bypass (without decreasing the TTL) while others to decrement TTL by 1?
There was a problem hiding this comment.
This case can be handled by the PropagationFilter: https:/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#tagpropagationfilter, which will be added in the next PR.
* Add initial spec for DistributedContext.Entry * Remove WIP TagMap specs. * Add Entry related terminology. * Add description of supported operations and types.
Updates #41 and #46.
Most are a copy-and-paste from https:/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#tag by renaming
Tag*->Entry*.Next PR will be
TagMap->DistributedContext.Followed open-telemetry/opentelemetry-java#11 (comment):
/cc @c24t @dinooliva @mayurkale22 @rghetia