Change `string ISpan.GetBaggageItem(string key)` to `bool ISpan.TryGetBaggageItem(string key, out string value)`

[gideonkorir]

Based on comments in 44 can we change the API to make it explicit that the key could be missing without requiring the client to check for nulls.

[cwe1ss]

I wonder if this should actually be a change or just an additional method. It's quite common in .NET to have both variants (e.g. int.Parse vs int.TryParse). The difference in most of these cases is though that the regular method throws an exception which we definitely don't want here.

[ndrwrbgs]

Generally speaking, I'd prefer to only offer the exception-less style. Often times developers only use the exception-style because they're trying to do something quick-and-dirty and pre-latest C# language spec, using the try flavor required at least 1 extra line of code. By only offering the try version callers have to be explicit if they want to throw or swallow failure rather than implicit.

TL;DR I would vote for changing it rather than adding it.

[cwe1ss]

Out of curiosity (I'm still pretty new to this), what's your use cases where having TryGetBaggageItem would simplify code? Is it "if this baggage item doesn't exist, set it"?

If we only had bool TryGetBaggageItem(string key, out string value) we could no longer use the ??-operator:

// Instead of
string sessionId = span.GetBaggageItem("SessionId") ?? "<none>";

// We would have to write
if (!TryGetBaggageItem("SessionId", out string sessionId) {
    sessionId = "<none>";
}

[gideonkorir]

@cwe1ss it's because null can be a valid value, using TryGetBaggageItem allows us to know whether or not the key actually exists in the baggage item.

Semantically TryGetBaggageItem means the key/value could be missing and that the client should expect that however GetBaggageItem doesn't communicate that as part of the api.

[cwe1ss]

I understand. But it's not really a good idea to add baggage with a null/default value as that's transferred via the wire and therefore unnecessary overhead. There might even be tracers who ignore such values on SetBaggageItem so relying on this seems to be risky.

[gideonkorir]

@cwe1ss agreed propagating null isn't a good idea & you can find ways to work around that. What I found as a user is I still required an if statement to prevent NPEs e.g. my flow basically was:

string value = span.GetBaggateItem(key);
if(!string.IsNullOrWhitespace(value)){
 //do something with value
}

And yes your use case makes lots of sense; I might have a downstream service that checks if the key is set it propagates the value otherwise it adds it's own key (Similar use-case to sampling flags)

[cwe1ss]

Well, but here it gets complicated because we can't reliably dictate tracer behavior so we can't reliably say whether or not the tracer stores null values. So if you only have TryGetBaggageItem and if you want to make sure you don't work with null values, you still have to check it - so you end up with code that's more complex than your example IMO:

if (span.TryGetBaggageItem(key, out string value) && !string.IsNullOrWhitespace(value)) {
  // do something with value
}

[gideonkorir]

@cwe1ss again agreed but I prefer the second notion more because to me it tells me that the key is in the baggage item and that the value is null. In the other case I just know that the value is/isn't null

[cwe1ss]

I think we have to come to an agreement about whether or not tracers are allowed to ignore keys with null-values. I've just asked a question about this in https://gitter.im/opentracing/public to get some opinions.

If tracers are allowed to ignore them, then asking for existence of a key just isn't useful/correct anymore as there's no longer a semantic difference between a non-existing key and a key with a null-value.

[cwe1ss]

There has only been one response regarding Jaeger - it does NOT store keys with null values.

So if we would change this API, the following call would return false, which is very confusing/misleading IMO.

span.SetBaggageItem("userId", null);
bool exists = span.TryGetBaggageItem("userId", out string userId); // returns false

I therefore think we should not change the current API.

Anyone still thinks this should be changed?

[ndrwrbgs]

Just a note - the title of this is misleading as ISpanContext doesn't have a GetBaggageItem(string)

[dmccaffery]

If null is an invalid value for SetBaggageItem, then the value should be [NotNull] and should throw an ArgumentNullException.

span.SetBaggageItem("userId", null); // should throw ArgumentNullException if null values are actually invalid

bool exists = span.TryGetBaggageItem("userId", out string userId); // returns false

I agree that the API as it stands today is misleading and does not work the way other APIs in the framework itself operate. To follow very common patterns with key/value dictionaries, the following should be used:

var value = span.GetBaggageItem("userId") // should throw KeyNotFoundException
span.TryGetBaggageItem("userId", out var value) // should return false and is a pure method

https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.keynotfoundexception?view=netcore-2.2