Runtime: Add a per Activity API to set the default ActivityIdFormat

[davidfowl]

We have a new standard and we should be using it by default in .NET Core 3.0 applications. Today the default IdFormat is Hierarchical instead of W3C. Was that intentional?

EDIT: API Proposal:

C# public partial class Activity { public void SetIdFormat(ActivityIdFormat idFormat); }

cc @lmolkova @SergeyKanzhelev @noahfalk

EDIT: Changed spelling to SetIdFormat

[stephentoub]

IIRC we discussed this with Vance and he made the case for not changing the default. @bartonjs may remember the details. There's probably also a recording of the API review (@terrajobst?)

[davidfowl]

It would like to revisit that decision. We have 2 flags:

C# Activity.DefaultIdFormat Activity.ForceDefaultIdFormat
Seems like it should be reaosnable to change the default id format without forcing it. This way we can generate the standard WC3 format by default (which is new and shiny and standard) and works without us having to tell customers to hardcode this static.

[noahfalk]

My 2 cents - The runtime is generally conservative about back-compat. I'd be hesitant to change a default unless we have evidence to say it is the better choice for the overwhelming majority of customers and we've given clear advance warning that the change is coming. I think W3C might hit that bar in the future, but I am skeptical it will hit that bar at the time we release .Net Core 3.0.

Likely when our customers upgrade to 3.0 a good number of them will updating different components of their distributed systems at different times. If we immediately opted them into W3C we would probably break compat with their existing 2.2 components or tools that haven't yet been updated, forcing them to delay their migration or modify their apps to remain on the hierarchical format. Only when all the software in their distributed system supports W3C will they be ready to enable it.

[davidfowl]

OK I have some thoughts:

• I'd like to say that we suppor the new wc3 trace context spec by default in ASP.NET Core applications. Today this requires that somebody changes their code (in the top most services or all services really) to set the new activity format.
• You'll forever have this problem of migration. If we don't change the default now, but change it later, it will have these exact same issues but with different version numbers (3.0 to 4.0 migration). What will be different then?
• How would you recommend a customer do the migration? Migrate services inside out changing the entry point in each of them to be the new format?

TL;DR I don't see how you avoid breaking them. My gut tells me we should change to the new format and let people use the APIs above to switch the ID back to the hierarchical format if they are broken.

I do want to call out that we did all of the work to preserve the incoming format so this won't just break even if we change the default. The root service would need to be changed to the w3c format to break. Even so, it may even be possible to update your 2.2 service to use the new System.Diagnostics.DiagnosticSource package to take advantage of the new format without going to 3.0.

https://github.com/dotnet/corefx/blob/c1b0a40964253b1bc48dd3be734eb2e1cf716db3/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L349-L394

I don't want to hack this into ASP.NET Core applications but we may have to if this is the decision.

[davidfowl]

@noahfalk it isn't possible to change this on the activity locally it seems. Start makes a decision based on various aspects and there isn't a non static way to change the it for a root activity. We would need to pass the format to Start to make this more local.

[noahfalk]

@davidfowl and I chatted about this a little offline. To capture it here I think these were our main conclusions but please correct as needed David : )

1) Migration is much easier when all the software components in your distributed system have support for W3C already baked in. Changing the default in 3.0 means the system will still be coexisting with not-yet-updated 2.1 or 2.2 components whereas changing the default in the future makes that much less likely.

2) ASP.NET wants to enable Activity tracking/distributed tracing by default, which will enable it in scenarios where no tracing was occuring before. If we know we are in a scenario where tracing wasn't previously enabled the back-compat concerns are substantially lower. ASP.NET is in position to know this but CoreFX is not.

3) In general ASP.NET has a lower bar for breaking changes and has more explicit configuration that customers are used to dealing with. Combined with point (2) it probably makes a lot of sense for ASP.NET to opt a swath of scenarios into W3C tracing and offer configuration. The best way to do that would be applying logic for root Activities that ASP.NET creates (and transitively it flows to all their children), which would require control per-Activity rather than process global statics.

@davidfowl did you want to make a PR for Activity that lets you control it without a static? I assume you have other work you are combining and testing this with and it seems like it would be a 10 line change to Activity. I think usage like this would match existing conventions for Activity where you call various setters prior to Start() and then Start() finalizes construction.

Activity a = new Activity("New web request");
a.IdFormat = ActivityIdFormat.W3C; // property setter would need to become public
                                   // with a little error checking
a.Start();                         // implementation would need to honor a pre-set format

I did a little code review and I didn't notice anything that would break if IdFormat were set to a non-unknown value prior to start being called. If there is a particular reason to prefer a Start(ActivityIdFormat) overload I'm open to that too, but it didn't seem to match the existing API convention as well.

[SergeyKanzhelev]

@noahfalk also Application Insights SDK will support some level of back compat. So if Request-Id was used in combination with AI SDK - it can fix up correlation where needed.

[davidfowl]

@noahfalk yes I can make that change. A couple of question:

• Do we need an API review to make this change? cc @terrajobst
• Is it weird that the property can be set anytime during the lifecycle of the activity? If it's set after Start, should it throw?
• Do we want to pass the format to Start instead?

[stephentoub]

Do we need an API review to make this change?

I think it'd be worth at least a brief discussion, in particular whether there's a better way to do it than making IdFormat settable.

If the IdFormat is explicitly set, does that override all of the logic here: https://github.com/dotnet/corefx/blob/c776c91e00a6c2ec7b3a2d3b30e6ccf4aad87b71/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L373-L383
?

Seems like passing it to another overload of Start would help to eliminate ambiguities, assuming that argument would override all of the logic currently there.

Another alternative would be a ctor argument.

If it's set after Start, should it throw?

I would think so, in particular because changing that would need to invalid a previously retrieved ID for that activity, right?

[davidfowl]

I like the constructor argument.

[davidfowl]

```C#
public partial class Activity
{
public Activity(string operationName, ActivityIdFormat defaultFormat);
}

As for behavior, I believe this should be preferred wherever DefaultIdFormat is used today (`_defaultIdFormat ?? Activity.DefaultIdFormat`). That means:

- If ForceDefaultIdFormat is set, it'll prefer the field over the static (we need a way to represent not set if this is the case, internally stored as a nullable?)
- Otherwise the usual logic runs and DefaultIdFormat is only used if the other cases aren't set.

i.e.

```C#
// Figure out what format to use.  
if (ForceDefaultIdFormat)
    IdFormat = _defaultIdFormat ?? DefaultIdFormat;
else if (Parent != null)
    IdFormat = Parent.IdFormat;
else if (_parentSpanIdSet)
    IdFormat = ActivityIdFormat.W3C;
else if (_parentId != null)
    IdFormat = IsW3CId(_parentId) ? ActivityIdFormat.W3C : ActivityIdFormat.Hierarchical;
else
    IdFormat = _defaultIdFormat ?? DefaultIdFormat;

_defaultIdFormat is the field set in the constructor of Activity.

This allows ASP.NET Core to set a new default while still retaining backcompat if an existing Hierarchical id is sent.

Open question: Do we need a constructor that forces the default format so this can be controlled outside of the static?

[SergeyKanzhelev]

Open question: Do we need a constructor that forces the default format so this can be controlled outside of the static?

For Application Insights we will probably want to force W3C and provide back compat with Request-Id ourselves.

So outside of the issue of multiple apps in a single appdomain - static should be enough for us

[davidfowl]

For Application Insights we will probably want to force W3C and provide back compat with Request-Id ourselves.

How would you do that?

So outside of the issue of multiple apps in a single appdomain - static should be enough for us

Is this for ASP.NET Core apps or any app?

[noahfalk]

I believe this should be preferred wherever DefaultIdFormat is used today

I propose a format set by the immediate caller should take precedence, always. Ie this always works regardless of statics or parent:

Activity a = new Activity();
a.SetFormatId(foo);
a.Start();
Debug.Assert(a.FormatId == foo);

If the caller wanted to defer to the parent similar to the DefaultFormatId static then they can still do that explicitly:

Activity a = new Activity();
if(Activity.Current == null)
    a.SetFormatId(foo);

I believe the implementation would then look like this:

if(IdFormat != ActivityIdFormat.Unknown)  // add this
{} // do nothing we are done
else if (ForceDefaultIdFormat)
    IdFormat = DefaultIdFormat;
else if (Parent != null)
    IdFormat = Parent.IdFormat;
else if (_parentSpanIdSet)
    IdFormat = ActivityIdFormat.W3C;
else if (_parentId != null)
    IdFormat = IsW3CId(_parentId) ? ActivityIdFormat.W3C : ActivityIdFormat.Hierarchical;
else
    IdFormat = _defaultIdFormat ?? DefaultIdFormat;

I'm updating my suggestion from setter to SetFormatId (I think now it matches the existing style, setter didn't)

void SetIdFormat(ActivityIdFormat format)
{
     if (_id != null || _spanIdSet)
     {
          NotifyError(new InvalidOperationException("Can not change format for an activity that was already started"));
     }
     IdFormat = format;
}

I don't have a strong opinion whether the format comes in as a constructor argument or Start() argument or SetFormatId(), but SetFormatId() seems to match the existing style.

[lmolkova]

My 2 cents:

• AppInsights will force W3C all the time. If customer app receives Request-Id, but not W3C - we'll start a new W3C id and stamp legacy Request-Id on the telemetry in addition to the new W3C Id so we can stitch telemetry on the UI.
  Statics are enough for us for all applications: ASP.NET Core or any other.

• This APIs is likely already a legacy and not going to be used going forward. Can we avoid exposing it at all?

@davidfowl I understand your hesitation, but can we entertain the idea of ASP.NET Core setting static DefaultIdFormat on host startup? Setting it to W3C and never attempting to read Request-Id header will do W3C enforcement. It has some disadvantages but hardcoding format and setting it on each activity seems to have the same issues.

[davidfowl]

@davidfowl I understand your hesitation, but can we entertain the idea of ASP.NET Core setting static DefaultIdFormat on host startup? Setting it to W3C and never attempting to read Request-Id header will do W3C enforcement. It has some disadvantages but hardcoding format and setting it on each activity seems to have the same issues.

No, I really don't want to affect any global state as a side effect of a non-static API. The customer would be force to do it in their main.

This APIs is likely already a legacy and not going to be used going forward. Can we avoid exposing it at all?

What is legacy? The format? We already expose it so I don't see a problem with letting this be an instance proeprty. We should consdier calling the old format Legacy instead of Hierarchal.

Setting it to W3C and never attempting to read Request-Id header will do W3C enforcement. It has some disadvantages but hardcoding format and setting it on each activity seems to have the same issues.

What issues?

[lmolkova]

What is legacy? The format? We already expose it so I don't see a problem with letting this be an instance proeprty. We should consdier calling the old format Legacy instead of Hierarchal.

Yes, the format is legacy, good idea to rename it before it ships. But also any new API we come up with to force the new format will become legacy the moment we ship it (i.e. Activity(string, format) or Start(format) or SetIdFormat) will never be used except in ASP.NET Core.

What issues?

All the activities created by ASP.NET Core will be forced to be W3C. All their children will be W3C as well - because the parent is W3C. This likely describes every Activity created in the process after host starts. Users are not able to change it unless you provide configuration (which is probably not an issue).

The interesting exercise is to think about the Activities created in background threads or during startup -unless global static DefaultIdFormat is set they will be hierarchical and it will be not consistent with what ASP.NET Core creates.

So tracing system or customer needs to set global DefaultIdFormat anyway and setting Id on each Activity is going to be redundant.

[davidfowl]

So the guidance is, they set this in main and forget about it? In the next major, we change that to be the default and have people remove that line?

[lmolkova]

I believe ASP.NET Core should enforce W3C. if we only rely on users setting it in the main - they will never do it and will get legacy format.

Assuming ASP.NET Core enforces W3C by using static DefaultIdFormat, we don't need new SetIdFormat API on the Activity. However if setting global statics is no-go, I'm ok with setting it on the Activity level.

BTW, I like SetIdFormat more than constructor and Start parameter because then it could be changed by user or tracing system (if it is ever needed) in OnActivityImport callback. With constructor or Start argument, it is fully controlled by the instrumentation in the framework.

[davidfowl]

I believe ASP.NET Core should enforce W3C. if we only rely on users setting it in the main - they will never do it and will get legacy format.

Agreed.

Assuming ASP.NET Core enforces W3C by using static DefaultIdFormat, we don't need new SetIdFormat API on the Activity. However if setting global statics is no-go, I'm ok with setting it on the Activity level.

Yes it's a no go.

BTW, I like SetIdFormat more than constructor and Start parameter because then it could be changed by user or tracing system (if it is ever needed) in OnActivityImport callback. With constructor or Start argument, it is fully controlled by the instrumentation in the framework.

OK so it's ok up until you call Start(). I'll update the proposal. What about the other open questions? Does this override everything? What if the parent id the legacy format and I set W3c? What does it do?

[lmolkova]

Does this override everything? What if the parent id the legacy format and I set W3c? What does it do?

From API simplicity it should override anything, otherwise it becomes very confusing.

I have an alternative proposal I'd like to entertain:
For tracing scenarios, setting Id format on the Activity that has local parent does not make sense. If local parent and child Activities have different Id formats - something is broken.

If there is a remote W3C parent - we are good (ASP.NET Core should not even attempt to read Request-Id).

So, we only need to call SetIdFormat when there is no remote W3C parent and it only makes sense to call this for the 'incoming' request instrumentation.

So since we only need to call it in the same case when we call SetParentId, why don't we overload this method instead of creating new API?

I.e. have SetParentId(string id, ActivityIdFormat defaultIdFormat) that applies defaultIdFormat when id==null

        public Activity SetParentId(string parentId, ActivityIdFormat defaultIdFormat)
        {
            if (Parent != null)
            {
                NotifyError(new InvalidOperationException($"Trying to set {nameof(ParentId)} on activity which has {nameof(Parent)}"));
            }
            else if (ParentId != null || _parentSpanIdSet)
            {
                NotifyError(new InvalidOperationException($"{nameof(ParentId)} is already set"));
            }
            else if (string.IsNullOrEmpty(parentId))
            {
                NotifyError(new ArgumentException($"{nameof(parentId)} must not be null or empty"));
            }
            else
            {
                _parentId = parentId;

                // this is the proposal
                if (_parentId == null) {
                    IdFormat = defaultIdFormat;
                }
            }
            return this;
        }

[davidfowl]

It's a bit weird to call it with a null id. This also looks expensive to call (all of those branches) when there's no parent. I'm not a fan of adding more to the hot path, Activity turns out is already quite heavy and we're attempting to chip away at it.

[lmolkova]

ok, so for the sake of simplicity, let SetIdFormat() override anything else.

[davidfowl]

Now I also want this API so I can unit test this thing properly 😄

[terrajobst]

Video

Personally, I'd prefer this being a property but it seems folks on the thread have strong opinion that this should be method, which is also accetable.

If that's a method it should be called SetIdFormat.

C# public void SetIdFormat(ActivityIdFormat idFormat);

[davidfowl]

@lmolkova do you have any cycles to get this in next week?

[lmolkova]

@davidfowl yeah, I'll do it.