url encode baggage keys and values #233

tsloughter · 2021-03-24T14:42:27Z

Fix for #224 and #225

This may not be completely ready yet but opening now for review.

Possible changes needed:

Are keys supposed to be percent encoded
How to handle metadata? It may just be part of the value and thus nothing to do (except that I don't think the ? and ; should be percent encoded if that is the case so even if it is just part of the value it may need updates to act properly.)

Baggage spec: https://w3c.github.io/baggage/

codecov · 2021-03-24T14:43:05Z

Codecov Report

Merging #233 (84615da) into main (3463415) will increase coverage by 0.80%.
The diff coverage is 63.80%.

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   34.79%   35.59%   +0.80%     
==========================================
  Files          37       37              
  Lines        3052     3149      +97     
==========================================
+ Hits         1062     1121      +59     
- Misses       1990     2028      +38

Flag	Coverage Δ
api	`61.48% <63.80%> (+0.37%)`	⬆️
elixir	`16.19% <9.52%> (-2.15%)`	⬇️
erlang	`35.58% <67.00%> (+0.93%)`	⬆️
exporter	`19.48% <ø> (ø)`
sdk	`77.14% <ø> (-0.37%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ps/opentelemetry_api/lib/open_telemetry/baggage.ex	`33.33% <0.00%> (-26.67%)`	⬇️
apps/opentelemetry_api/src/otel_ctx.erl	`72.41% <57.14%> (-6.76%)`	⬇️
apps/opentelemetry_api/src/otel_baggage.erl	`70.43% <67.74%> (-11.05%)`	⬇️
apps/opentelemetry/src/otel_batch_processor.erl	`66.32% <0.00%> (-2.05%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3463415...84615da. Read the comment docs.

garthk

Converting chardata baggage to binaries only in otel_propagator:text_map_inject/1 leaves us open to confusion if people mix binaries and chardata in their arguments to otel_baggage:set/1 and set/2 and then call get_all/0. Are you up for converting in the set functions? Failing that, erroring with badarg on chardata?

~~I'm blocked on waiting for Erlang 23.2.7 to install so I can continue testing, but next I'll be checking for the whitespace decoding.~~

garthk · 2021-03-24T22:30:45Z

apps/opentelemetry/test/opentelemetry_SUITE.erl

+    otel_baggage:set(<<"key-1">>, <<"value=1">>),
+    otel_baggage:set(<<"key-2">>, <<"value-2">>),


If we're still accepting UTF-8 chardata as arguments, perhaps we could reflect that in the tests?

Suggested change

otel_baggage:set(<<"key-1">>, <<"value=1">>),

otel_baggage:set(<<"key-2">>, <<"value-2">>),

%% We prefer UTF-8 binary keys and values...

otel_baggage:set(<<"key-1">>, <<"value=1">>),

%% ... but still support UTF-8 character lists:

otel_baggage:set("key-2", "value-2"),

Unfortunately the clean way of declaring the keys literally would be <<"key-1"/utf8>> since otherwise byte values > 255 get truncated

Yeah, the smiley face [128512] for example would need encoding… as much as I'd prefer to avoid crashing out, it's more straightforward to throw a bad match than stack up on “friendly” special case handling.

In case I'm missing your point, though, <<"😀"/utf8>> would be a great example for people mining our tests for usage hints. I was surprised to see <<"😀">> == <<0>>.

Good point @ferd

@garthk yea, can't throw a bad match. Instrumentation is not program logic, we will not crash. There are currently still places we can crash that need to be fixed. You should be able to do all kinds of accidental nonsense and not crash.

Fair enough. Next-best: either dropping chardata arguments, or quietly converting them to binary.

apps/opentelemetry_api/src/otel_baggage.erl

garthk · 2021-03-24T23:37:42Z

Whitespace strips OK, but we need to split off the opaque metadata from each value. Using the example from the spec:

:otel_propagator.text_map_extract([{"baggage", "key1=value1;property1;property2, key2 = value2, key3=value3; propertyKey=propertyValue"}]) 
:otel_baggage.get_all()
# result:                 
%{
  "key1" => "value1;property1;property2",
  "key2" => "value2",
  "key3" => "value3; propertyKey=propertyValue"
}
# expected:
%{
  "key1" => "value1",
  "key2" => "value2",
  "key3" => "value3"
}

The spec doesn't say we SHOULD preserve the metadata to pass it along late; let's just drop it?

tsloughter · 2021-03-25T16:18:51Z

@garthk I added the functions for explicit context passing to baggage to this PR 9276fa5

ferd · 2021-03-25T18:11:50Z

apps/opentelemetry_api/src/otel_baggage.erl

+decode_value(Value) ->
+    percent_decode(string:trim(unicode:characters_to_binary(Value))).
+
+%% TODO: call `uri_string:percent_decode' and remove this when OTP-23 is


Should we deal with any licensing and copyright here?

tsloughter · 2021-03-25T21:38:38Z

@garthk if we are going to parse out the metadata I don't see a reason to drop it. But this will mean the value will become a 2-tuple:

-type entry() :: #{key() => {value(), metadata()}.

garthk · 2021-03-25T21:58:36Z

It'd make for some entertaining API choices. :)

Least surprising there might be set/2 taking "value",{"value", "metadata"}, and {"value", nil} as arguments, and get_all/0 always returning the 2-tuple even if the metadata is absent %{"key" => {"value", nil}} so people don't get fooled into thinking it'll always be a straight binary-binary map. If we're trying to avoid crashes in our code, because instrumentation, may as well also help avoid crashes in the code people write to work with our code.

tsloughter · 2021-03-25T22:17:14Z

Right. I was thinkingi the same. Except instead of nil it'll be [].

garthk · 2021-03-25T22:20:49Z

Oh, of course, because metadata is its own list of bare keys and key-value pairs.

tsloughter · 2021-03-28T11:35:48Z

I've added support for metadata. I'd have made it a separate PR but realized this one still had open issues so added it on, hopefully aside from fixes this is it and I won't let it grow anymore :)

garthk · 2021-03-28T21:16:34Z

apps/opentelemetry/test/opentelemetry_SUITE.erl

+    ?assertEqual(#{<<"key-1">> => {<<"value=1">>, []},
+                   <<"key-2">> => {<<"value-2">>, [<<"metadata">>]}}, otel_baggage:get_all()),


Could we show the {<<"metadata_key">>,<<"metadata_value">>} form, also?

garthk · 2021-03-28T21:17:30Z

apps/opentelemetry/test/opentelemetry_SUITE.erl

+    %% TODO: should the whole baggage entry be dropped if metadata is bad?
+    %% drop bad metadata (the `1').


Drop the bad metadata value only, I think.

tsloughter · 2021-04-04T11:56:29Z

@ferd @garthk anything more that needs to be done before I merge this? (aside from the failing CI of course)

tsloughter · 2021-04-04T11:56:59Z

@ferd @garthk anything more that needs to be done before I merge this? (aside from the failing CI of course)

tsloughter requested a review from a team March 24, 2021 14:42

github-actions bot added language-erlang scope-api scope-sdk labels Mar 24, 2021

garthk reviewed Mar 24, 2021

View reviewed changes

tsloughter force-pushed the baggage-encoding branch from 0dab7cd to 9276fa5 Compare March 25, 2021 15:25

ferd reviewed Mar 25, 2021

View reviewed changes

tsloughter force-pushed the baggage-encoding branch from 871426e to da13b97 Compare March 25, 2021 20:13

github-actions bot added the language-elixir label Mar 25, 2021

garthk reviewed Mar 28, 2021

View reviewed changes

tsloughter force-pushed the baggage-encoding branch from da1f036 to 8ce95b9 Compare March 29, 2021 17:51

tsloughter requested a review from ferd April 2, 2021 22:28

tsloughter force-pushed the baggage-encoding branch 2 times, most recently from b98d946 to 70d1ac4 Compare April 4, 2021 12:49

ferd approved these changes Apr 4, 2021

View reviewed changes

tsloughter force-pushed the baggage-encoding branch 2 times, most recently from 3f23fb6 to 99d475d Compare April 4, 2021 17:02

tsloughter added 3 commits April 4, 2021 11:05

url encode baggage keys and values

830ab88

remove gitter badge, we are on CNCF slack now

f9b4bfa

add optional explicit context argument to baggage functions

66c1cb6

tsloughter added 8 commits April 4, 2021 11:05

vendor in percent_decode so OTP 21 and 22 are supported

2650265

add new api functions to the elixir module

a30a792

support baggage value metadata

f36b846

add to metadata test case with k/v property

f2269d4

convert charlist strings to binaries when adding baggage

a92f488

fix elixir baggage tests

7083731

fix typespec of baggage set function

89cb298

fix typespec of baggage set function

84615da

tsloughter force-pushed the baggage-encoding branch from 0f8bee3 to 84615da Compare April 4, 2021 17:07

tsloughter merged commit a9fb88a into open-telemetry:main Apr 4, 2021

This was referenced Apr 5, 2021

otel_propagator doesn't escape values #225

Closed

otel_propagator round trip turns binary keys and values into character lists #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url encode baggage keys and values #233

url encode baggage keys and values #233

tsloughter commented Mar 24, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

garthk left a comment •

edited

Loading

garthk Mar 24, 2021

ferd Mar 25, 2021

garthk Mar 25, 2021

tsloughter Mar 25, 2021

garthk Mar 25, 2021

garthk commented Mar 24, 2021

tsloughter commented Mar 25, 2021

ferd Mar 25, 2021

tsloughter commented Mar 25, 2021

garthk commented Mar 25, 2021

tsloughter commented Mar 25, 2021

garthk commented Mar 25, 2021

tsloughter commented Mar 28, 2021

garthk Mar 28, 2021

garthk Mar 28, 2021

tsloughter commented Apr 4, 2021

tsloughter commented Apr 4, 2021

		otel_baggage:set(<<"key-1">>, <<"value=1">>),
		otel_baggage:set(<<"key-2">>, <<"value-2">>),

		?assertEqual(#{<<"key-1">> => {<<"value=1">>, []},
		<<"key-2">> => {<<"value-2">>, [<<"metadata">>]}}, otel_baggage:get_all()),

		%% TODO: should the whole baggage entry be dropped if metadata is bad?
		%% drop bad metadata (the `1').

url encode baggage keys and values #233

url encode baggage keys and values #233

Conversation

tsloughter commented Mar 24, 2021

codecov bot commented Mar 24, 2021 • edited Loading

Codecov Report

garthk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garthk commented Mar 24, 2021

tsloughter commented Mar 25, 2021

Choose a reason for hiding this comment

tsloughter commented Mar 25, 2021

garthk commented Mar 25, 2021

tsloughter commented Mar 25, 2021

garthk commented Mar 25, 2021

tsloughter commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsloughter commented Apr 4, 2021

tsloughter commented Apr 4, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

garthk left a comment •

edited

Loading