otel_propagator doesn't escape values

[garthk]

This strikes me as dangerous:

OpenTelemetry.Baggage.clear()       
OpenTelemetry.Baggage.set("k1", "v1")
OpenTelemetry.Baggage.set("k2", "v2,k1=v3")
OpenTelemetry.Baggage.get_all()                  #  %{"k1" => "v1", "k2" => "v2,k1=v3"}
text_map = :otel_propagator.text_map_inject([]) 
OpenTelemetry.Baggage.clear()  
:otel_propagator.text_map_extract(text_map)
OpenTelemetry.Baggage.get_all()                  # %{'k1' => 'v1', 'k2' => 'v2'} <------ surprise!

It might also be a little too trusting about keys. I imagine shoving JSON into a baggage value will get messy.

(I previously raised binaries turning into charlists in #224.)

[tsloughter]

I think it might be restricted to not allow commas. But means we need to drop that pair. So either way the current functionality is wrong :) https://w3c.github.io/baggage/#definition

[tsloughter]

Yea, both key and value can't have separators in them.

[tsloughter]

Oh, nm, values are url encoded (but seems keys aren't).

[garthk]

The spec strikes me as a little loose. I reckon implementations MUST escape commas, semicolons, and equals signs in the keys and values because the alternatives are clearly worse: crashing, dropping keys and values, or corrupting the values for other keys. How do we push that along? Surely they'll at least go for a SHOULD.

In Elixir you can augment the determination of which characters are URI-escaped with the second argument to URI.encode/2:

iex(21)> URI.encode("x=foo,bar;\"", & (&1 not in [?,, ?=, ?;]) and URI.char_unescaped?(&1))
"x%3Dfoo%2Cbar%3B%22"

[tsloughter]

Pretty sure they (the key/value pair or the whole thing if its in the process of being parsed from a header) just get dropped because they aren't allowed.

[bryannaegele]

Yeah, I'd opt for a simple regex and just drop if it fails for performance reasons.

[garthk]

I find silent failure eg. dropping values we don't like makes for frustrating troubleshooting. The spec demands we encode anyway, and adding an extra regular expression check surely can't make it faster. URI encoding the delimiters as well as (say) backslashes is a cheap and easy fix, no? Anyhow if you could upgrade from "yeah nah" to "yeah ok I guess", to use the Aussie expressions, I'd very much appreciate it.

[garthk]

Checked the spec again, and I'm:

• Even more sure URI-encoding all UTF-8 binary values is the right thing to do, hence %-encoding commas and equals signs
• Less sure about the keys, which are distinct from the values in that they don't ask for URI-encoding and emphasise the need for them to be RFC 2616 tokens

Lucky for us, tokens permit ., so our semantic attributes eg. enduser.id MAY be baggage keys.

       CTL            = <any US-ASCII control character
                        (octets 0 - 31) and DEL (127)>
…
       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

For dropping bad keys only, is there an Erlang equivalent to the Elixir community habit of compiling in warnings for dev and test, but not for prod? Then we could at least tell them why their Phoenix style nested keys with square brackets disappeared.

[tsloughter]

For values, right, we agree there. I think we need to send a patch to OTP that includes the new function we'll have to be added into Otel to do this.

I think uri_string:transcode assumes you are encoding a query string and thus don't want = encoded. I checked Python and we need to add a quote function like python has in urllib:

>>> urllib.parse.quote('hello=world')
'hello%3Dworld'

Ideally I think transcode is a bad name in uri_string and percent encoding the whole query string at once is incorrect but will have to take that up with them first, maybe I'm missing something.

And I think it should be named percent_encode to match uri_string:percent_decode.

[tsloughter]

And I don't know of a pattern for that logging stuff in Erlang.

I was going to say we could add a define {d, 'NO_DEV_LOGS'} in the prod profile and use that to wrap the function that logs these messages but all deps are compiled in the prod profile, meaning you would only get this log when working on OpenTelemetry itself, which isn't what you are looking for.

[tsloughter]

Did some more digging and uri_string has another function that also does encoding and does get it right,

uri_string:compose_query([{"a", "b="}]).
"a=b%3D"

So it just needs to export the form_urlencode function so a user can url encode any string they want.

And for now we'll just vendor in that function.

[garthk]

Great find! uri_string:compose_query/1 and uri_string:dissect_query/1 look great, and work with binaries.

[tsloughter]

If you are referring to for baggage, compose_query can't be used because baggage has the concept of metadata.

But... hm, now that I think about it (after doing the work of vendoring in the encode function and using it, hehe) we could turn the value into the value+metadata and then use compose_query... guess I'll try that now too.

[garthk]

Oh, I'm just grabbing for whatever code I can get for free. There'll always be more work, eg. appending the metadata block when encoding, and trimming the optional white space (OWS) around keys¹ and values² when decoding. Unless we write something 100% custom, but I think I'd rather spend the time elsewhere.

¹ https://w3c.github.io/baggage/#key
² https://w3c.github.io/baggage/#value

[tsloughter]

So, nm, can't use compose_query since it will url encode the ; metadata separator and in at least the java implementation it searches for ; before decoding, so that would break if we were encoding those.

[garthk]

Yeah, nah, we'd need to append the ; and metadata ourselves while encoding, and split at ; ourselves and dissect_query only the first half while decoding. No biggie.

[tsloughter]

@garthk just realized I was being stupid, for logs we don't want in production we simply use the debug level :)

[tsloughter]

Closing now that #233 is merged. But I think we need a new issue about a) adding debug logs in multiple places b) adding a regex to verify a key is within the allowed chars and dropping it if it isn't.