resources: define string values and keys as urlencoded #505
Conversation
tsloughter
requested review from
arminru,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
| @@ -37,6 +37,9 @@ Required parameters: | |||
| - a collection of name/value attributes, where name is a string and value can be one | |||
| of: string, int64, double, bool. | |||
|
|
|||
| The key and value, if value is a string, are trimmed, url encoded strings and | |||
There was a problem hiding this comment.
This seems like an implementation detail / something the exporter should do if required. If I have an exporter that supports arbitrary strings as keys and values then I don't want to urlencode -- I might even have to urldecode in the exporter.
There was a problem hiding this comment.
It is also important for when reading in from the environment, or other sources.
If the value is simply defined as urlencoded then it makes dealing with these simpler and defined all at once.
There was a problem hiding this comment.
Well, not really IMHO. If we just define that values are URL-encoded then I will still need to urldecode the environment variable to avoid double-encoding when I pass the parsed values to the resource constructor.
There was a problem hiding this comment.
If it is defined as url-encoded then you know not to do an encoding so there is no need to decode.
There was a problem hiding this comment.
So who is responsible for doing the encoding?
There was a problem hiding this comment.
Ok, so just adding a MUST be url encoded strings?
--
Edited to fix SHOULD to MUST
There was a problem hiding this comment.
I agree with @Oberon00 , this seems misplaced. In most cases the attributes will be transmitted either via proto or via JSON, neither of which require URL encoding, which only increases the size. The situation with reading ENV vars is an example of another externalized representation, which also happens to have other rules (like it's a comma-separated list). So URL-encoding may be applied to the definition of that externalized representation, not to the API or how the values are represented internally.
There was a problem hiding this comment.
"SHOULD be" is still bad. If that is true, then nothing changes, i.e. that statement has no effect. The question the spec should answer is who (implementation? caller?) should do what (encode? decode?) and when (at export? when creating resources? at the backend?).
There was a problem hiding this comment.
My reply is about the "be" though, should vs must is not my issue.
|
@open-telemetry/specs-approvers please review this. We need to make progress. |
|
If correlations switch to using IETF dictionary, then. perhaps environment variables should be expressed in this form too. |
|
@jmacd IETF dictionary? |
|
So it started to seem like the consensus was on only having OS environment variables use url-encoding. Is this the case? I can close this. |
carlosalberto
added
the
spec:resource
reyang
added
area:sdk
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closing. |
As discussed in the last Spec SIG meeting Resource will use urlencoded strings like CorrelationContext for keys and values.
This will show its usefulness most in parsing the OS environment variable with Resource attributes, but there isn't a spec for that yet (I think?).