Telemetry Baggage proposal

[samsp-msft]

Baggage Proposal

Introduction

Baggage is an API to enable passing key value pairs between contexts for web calls. There are two related specs:
a) W3C baggage propogation https://www.w3.org/TR/baggage/
b) OpenTelemetry baggage API https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/baggage/api.md
https://opentelemetry.io/docs/specs/otel/baggage/api/

Existing Baggage API

.NET has APIs on Activity for managing baggage:

• public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string,string?>> Baggage { get; }
• public System.Diagnostics.Activity AddBaggage (string key, string? value);
• public System.Diagnostics.Activity SetBaggage (string key, string? value);

The premise is that baggage can be added to an activity. Child activities will inherit the baggage fron the parent, but with a variation of copy on write semantics so that changes to the child are not replicated back to the parent.

Problems with the existing Baggage Support

• The Baggage API on Activity doesn't work as its specified to - Setting a baggage with the same key as an existing item will not override the value, it just adds a new value.
• Baggage is tied to Activity - this seems to be more of a theoretical problem than specific functionality is broken because of it. An implicit root activity is created in .NET even if it is not flagged to be recorded.
  • OTel specs baggage to be separate from Tracing, but given .NET has Activity built-in, the concern seems to be more about spec divergance, rather than limiting functionality
• OTel has the concept of scopes for baggage - the scenario for scopes is to be able to create a new scope that does not inherit the baggage of the parent scope, so the parent baggage is not passed to a downstream service when that service is not trusted with that baggage info.
• W3C standard changed the header to "baggage". ASP.NET understands both baggage and Correlation-Context but http client LegacyPropagator only understands Correlation-Context so is sending the wrong header.
• W3C spec for baggage enables multipe related values to be attached to a property name, but are not the value of the property. #5677
• Encoding issues
  • [OTel.NET] Baggage encoding should be using % encoding for spaces. #5687 showed that fixing encoding is more complicated than it first seems.
  • [BaggagePropagator] Do not encode and decode keys #5479
  • [baggage] space encoding in baggage item value when injecting baggage #5260
• Updated OTel Spec Specify allowed characters for Baggage keys and values open-telemetry/opentelemetry-specification#3801

Goals

• Introduce API changes to .NET runtime/extensions that support Baggage in a W3C compliant way and APIs that are sympathetic to the goals expressed in the OTel specifications
• Keep the 3 existing APIs on Activity and map them to the new functionality
• Support propogation
  • Reading using both the baggage and legacy Correlation-Context headers,
  • writing using the baggage header as part of HttpClient (change in functionality)
• Enable contexts to be created to enable more control over baggage propogation and data hiding
• Enable the implementation to be wrapped/used by the OTEL.NET library

Non-goals

• Port the existing support from OTel.NET to runtime/extensions

Related Issues

.NET Runtime

• Support W3C Baggage propagation without Activity #103174
• [API Proposal]: System.Diagnostics.ActivityContext.Current ability or similar #86966
• Allow creation of a root Activity when Activity.Current is not null #65528
• Duplicated activity baggage when calling SetBaggage on parent and current activity #59496
• Support W3C Baggage proposed standard in HttpHandlerDiagnosticListener #45496

OpenTelemetry .NET

• BaggagePropagator handle baggage properties #5677
• Baggage propagation is not possible for instrumentations without depending on OpenTelemetry.Api package #5667

Concepts for discussion

This is no where near final, but I want to put a stake in the ground that we can then argue over :-)

immutability

The OTel spec for baggage talks about immutability, which has led Java to create a baggage API using the builder pattern. For example

    // Access current baggage with Baggage.current()
    // output => context baggage: {}
    Baggage currentBaggage = Baggage.current();
    System.out.println("current baggage: " + asString(currentBaggage));
    // ...or from a Context
    currentBaggage = Baggage.fromContext(current());

    // Baggage has a variety of methods for manipulating and reading data.
    // Convert to builder and add entries:
    Baggage newBaggage =
        Baggage.current().toBuilder()
            .put("shopId", "abc123")
            .put("shopName", "opentelemetry-demo", BaggageEntryMetadata.create("metadata"))
            .build();
    // ...or uncomment to start from empty
    // newBaggage = Baggage.empty().toBuilder().put("shopId", "abc123").build();
    // output => new baggage: {shopId=abc123(), shopName=opentelemetry-demo(metadata)}
    System.out.println("new baggage: " + asString(newBaggage));

The problem with this approach is that it isn't really compatible with the existing Activity.SetBaggage() set of methods, that I think we should endeavor to keep compatibility with.

The scenario where this is important is when a new baggage context is created, which inherits from the parent context. If there are changes to the parent context after the fork, should those be visible to the child. Eg I set a value in Actviity1, create a child Activity2, and then in another task update Activity1. I think that Activity2 should have a snapshot of the state when it was created.

Currently we try to be clever, and have a heirarchy of baggage state. I am thinking that we can probably achieve a good experience using copy semantics for child contexts. If baggage is a list of key value pairs, which are strings, and strings are immutable in .NET, then copying the list will not be excessive in memory consumption as its just pointers to the string values, which can't change. Within a context, values can be added and changed. Only when a new context is created is a new snapshot created.

The other altrenatives would be:

• always create a copy on every write - is inefficient unless you use a builder pattern, even then unrelated code is going to cause duplication
• have pointers to child baggage contexts and use change notifications to force duplication when non-leaf edits are made

Context storage

The OTel specs are based around the idea of a context, and then baggage and spans are stored as part of the context. In .NET we already have Activity as a built in type, and its automatically created by ASP.NET and (hopefully other infrastructure). I don't think we should go back and re-imagine having Activity as a child of a new context, so we should work with what we have.

My suggestions

• Within the scope of an Activity, store the baggage(s) in the activity.
• if not in an activity, then use an AsyncLocal

Why not just always use an async local? The scenarios for baggage will have a very high overlap with Activity and as most access will be via Activity, and Activities will need to snapshot the baggage on creation, we might as well store the data there.

Proposal

• Add a Baggage class to store baggage data

  • Has Add/get/set functionality
  • Also optional metadata string
  • implements IEnumerable<KeyValuePair<string,string?>> so it can be returned by Activity.Baggage

• Add a BaggageContext static class to manage baggage contexts

  • stores data in Activity or Asynclocal if not in an Activity context
    • why? - so we don't need to have as many context's flying around
  • static BaggageContext.Current would return the current baggage
    • Activity.Baggage would return Baggage.Current
  • Has Push/Pop semantics for managing the contexts, so new ones can be created with child activities or on demand
    • Activity creation will create a new context, using the parent activity (if avaiable) for baggage parent
  • Baggage can be dependent on their parent or fresh
    • When created from a parent, it does a shallow copy of the parent baggage
    • Fresh context will not pull through any values from the parent context

• Add a new W3C propogator that reads both baggage and Correlation-Context but serializes using the baggage header and is compliant with standards for string serialization/escaping.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

[samsp-msft]

@tarekgh @lmolkova @CodeBlanch @noahfalk @cijothomas

Thoughts?

[tarekgh]

Why what OTel has is not enough and need to have something like this in the runtime?

[samsp-msft]

The reason this should go in the runtime is because:

• we are not following the standards in our propagation today - which should happen even if the application is not referencing OTel
• The current behavior for baggage APIs on Activity is broken
• We have docs in OTel to say not to use the Activity version of baggage, and only use the OTel libs

As said in open-telemetry/opentelemetry-dotnet#6144 (comment) we should fix this in the runtime, and then deprecate the functionality in the OTel libs.

[noahfalk]

Thanks for forging ahead on a tricky feature Sam! Mostly just questions at this point, I probably don't fully understand the proposal yet.

• In some places it sounded like we were trying to be compatible with the existing Activity Baggage APIs, and in other places it sounded like we were proposing breaking changes. If I understood right I think the proposal was "the APIs will still exist, but the behavior won't be the same". It would help to understand how impactful this is expected to be. Is it just a few apps doing corner-case things with the current API that will need to change their code or is it more broad?

• You mentioned that having a mutable baggage context that would create copies when forked. I wasn't sure which operations cause it to be forked?

• I wasn't sure if changes to Activity.Current are supposed to change Baggage.Current?

• If creating a new Activity forks the baggage, that suggests when I call activity.Baggage on different activity instances I might get different results. Elsewhere the proposal said that activity.Baggage would call Baggage.Current which suggests activity.Baggage won't change regardless which activity instance it is called on. I probably misunderstood something but that part seemed contradictory.

• It sounded like making the baggage mutable was partly a perf consideration and partly a compromise to retain more compat with the current Activity API. From the perf angle immutability means we copy-on-write vs. mutability is copy-on-fork. I'd guess we fork more often than we write so I'd guess the immutable copy-on-write might come out ahead if perf was the only factor. I wasn't sure how exactly immutability interfered with continuing to use the SetBaggage() API? For example could we just define that each time you call SetBaggage() we generate a new immutable instance of Baggage and then update Activity to refer to that new Baggage?

[samsp-msft]

In some places it sounded like we were trying to be compatible with the existing Activity Baggage APIs, and in other places it sounded like we were proposing breaking changes. If I understood right I think the proposal was "the APIs will still exist, but the behavior won't be the same". It would help to understand how impactful this is expected to be. Is it just a few apps doing corner-case things with the current API that will need to change their code or is it more broad?

The current Baggage APIs do not behave as the documentation states, in that child activities do not hide the values of parent activities when the same key is used. Similarly setting a null value does not clear the value from Baggage.

I am proposing that the new API would behave as the Baggage APIs have been documented to behave.

You mentioned that having a mutable baggage context that would create copies when forked. I wasn't sure which operations cause it to be forked?

Any operation that creates a child baggage will fork a copy of the parent baggage. This is so that subsequent writes to the parent baggage do not affect the child. We need to think about what happens if a task creates a new Activity and another task edits the baggage of the parent activity - what state should the child activity have?

If creating a new Activity forks the baggage, that suggests when I call activity.Baggage on different activity instances I might get different results. Elsewhere the proposal said that activity.Baggage would call Baggage.Current which suggests activity.Baggage won't change regardless which activity instance it is called on. I probably misunderstood something but that part seemed contradictory.

The state for baggage will be stored in the Activity, so that we don't need another AsyncLocal in most cases. The Activity.Baggage should be wired up so it gets the baggage for that Activity. I was probably confusing when I said it will Baggage.Current - the point is that it continues to return a baggage object that can be enumerated in the same way as before.

It sounded like making the baggage mutable was partly a perf consideration and partly a compromise to retain more compat with the current Activity API. From the perf angle immutability means we copy-on-write vs. mutability is copy-on-fork. I'd guess we fork more often than we write so I'd guess the immutable copy-on-write might come out ahead if perf was the only factor. I wasn't sure how exactly immutability interfered with continuing to use the SetBaggage() API? For example could we just define that each time you call SetBaggage() we generate a new immutable instance of Baggage and then update Activity to refer to that new Baggage?

It's going to depend on what the most common usage is of baggage. How often are people adding more baggage, and how many attributes do they add.

We could probably implement this with:

• Full immutability - each edit creates a new copy of the baggage, children can reference a specific instance - Java does this
• Change notifications - child baggage references the parent and forks its copy if the parent is changed - seemed overly complicated
• Child copies - child baggage shallow copies the parent baggage when its created - what I proposed
  There are probably other alternatives that could be used too.

[noahfalk]

I am proposing that the new API would behave as the Baggage APIs have been documented to behave.

Does that mean the APIs will completely ignore Baggage.Current, or you mean they will behave as currently documented only in scenarios where Baggage.Current isn't being used? For example in this code snippet:

Baggage.Current = <some_non_empty_baggage>
Activity.Current = null;
using(var a = Activity.Start("foo"))
{
    Console.WriteLine(a.Baggage.Count());
}

If the Activity APIs followed the current docs Baggage.Current would be irrelevant and 0 would be printed. Is the proposed behavior also to print 0, or its going to print something else, or my snippet makes no sense because Baggage.Current won't be usable like that?

Any operation that creates a child baggage will fork a copy of the parent baggage.

Which operations create a child baggage? It sounds like creating a new Activity would do that but it wasn't clear if that is the only operation that does.

[samsp-msft]

If the Activity APIs followed the current docs Baggage.Current would be irrelevant and 0 would be printed. Is the proposed behavior also to print 0, or its going to print something else, or my snippet makes no sense because Baggage.Current won't be usable like that?

Assuming that you setup some baggage in the current context - which following the spirit of your code you were doing in line 1.

When you start the foo activity, it should capture the current baggage context at that point.

IMHO your WriteLine would have a non-0 response.

What is more interesting is if the code becomes:

Baggage.Current.Set("name","value"); //<some_non_empty_baggage>

// snapshot the current context
var startContext = Baggage.Current;
Activity.Current = null;
using(var foo = Activity.Start("foo"))
{
    Console.WriteLine(foo.Baggage.Count());
}

//Set some more baggage via the activity
Activity.Current.AddBaggage("ActivityKey", "ActivityValue");

// Has this affected the current baggage?
Console.WriteLine(foo.Baggage.Count() == Baggage.Current.Count());

// Has this affected the snapshot that was created at the beginning?
Console.WriteLine(foo.Baggage.Count() == startContext.Count());