Change Activity Default IdFormat to W3C #37686

tarekgh · 2020-06-10T02:00:08Z

This change will make the default Activity IdFormat set to W3C instead of Hierarchical. As this a breaking change, we are providing a config switch to turn back te old behavior if needed.
Also, the change is moving the file LocalAppContextSwitches.Common.cs from the System.Private.CoreLib subfolder to the common source folder as it is used by multiple libraries.

This breaking change will be applied to .NET 5.0 only.

ghost · 2020-06-10T02:00:14Z

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
Notify danmosemsft if you want to be subscribed.

tarekgh · 2020-06-10T02:02:54Z

I marked this PR with new-api-needs-documentation tag although we don't have API changes, we have a new config switch that needs to get documented.

tarekgh · 2020-06-10T02:05:32Z

CC @shirhatti @davidfowl @reyang @chandramouleswaran

...raries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LocalAppContextSwitches.cs

tarekgh · 2020-06-11T02:18:48Z

Looks dotnet-runtime-perf (Performance Windows_NT x64 release coreclr net5.0) failure is unrelated according to the report https://dev.azure.com/dnceng/public/_pipeline/analytics/stageawareoutcome?definitionId=783 as it is failed more than 33 times today. I logged #37732 to track the failure.

noahfalk

Mostly looks good but I want to resolve two things:

Getting the correct scope on the new default
I want to ensure we are doing are due diligence in regards to breaking change notification and feedback. I think @shirhatti was looking into it but I don't recall if there was a conclusion? I am also going to loop in @richlander as well. I want to get a thumbs up from one of them.

src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs

shirhatti · 2020-06-15T08:49:19Z

~~Is there a way to set an AppContext switch without recompilation? I would assume not. If so, why introduce an AppContext switch for something when a public API already exists?~~

EDIT: Leaving my 🤦-worthy comment above. I just learned AppContext switches can be set via the runtimeconfig.json OR app.config.

Looking at the SocketsHttpHandler, it looks like they've introduced a corresponding an environment variable to go with each AppContext switch. Perhaps we should do something similar here?

EDIT: Are there any BCL guidelines on this? Should we introduce an env variable?

tarekgh · 2020-06-15T18:29:40Z

EDIT: Leaving my 🤦-worthy comment above. I just learned AppContext switches can be set via the runtimeconfig.json OR app.config.

right, editing the runtimeconfig.json will reflect on the app without recompiling.

Looking at the SocketsHttpHandler, it looks like they've introduced a corresponding an environment variable to go with each AppContext switch. Perhaps we should do something similar here?
EDIT: Are there any BCL guidelines on this? Should we introduce an env variable?

By default, the environment variable is not supported. It depends on the scenario of the configuration if you think there are scenarios the apps will not be able to set the switch in runtimeconfig.json and need the environment variable, then yes we can support the environment variable too. one example of that is we have a globalization invariant switch which we need to set it for the whole docker image and not only for one app, at that time the only solution to set the switch is by the environment variable.

tarekgh · 2020-06-15T18:35:43Z

dotnet/docs#18953

…tivityDefaultIdFormatToW3C

tarekgh · 2020-06-17T00:10:37Z

@noahfalk @shirhatti I have scoped the change to .NET 5.0 only. I have introduced the environment variable DOTNET_SYSTEM_DIAGNOSTICS_DEFAULTACTIVITYIDFORMATISHIERARCHIAL to be supported as a config switch too.

@shirhatti thankfully already filled the breaking change template dotnet/docs#18953 too. I edited this breaking change issue to reflect the latest info.

Please have a look and let me know if you have any more comments or we good to go?

noahfalk

Looks good to me modulo last few comments. Thanks @tarekgh and @shirhatti : )

noahfalk · 2020-06-17T09:12:13Z

...libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

@@ -19,6 +19,7 @@
    <DefineConstants Condition="'$(TargetFramework)' != 'netstandard1.1'">$(DefineConstants);EVENTSOURCE_ACTIVITY_SUPPORT</DefineConstants>
    <DefineConstants Condition="'$(TargetFramework)' != 'netstandard1.1' and '$(TargetFramework)' != 'netstandard1.3'">$(DefineConstants);EVENTSOURCE_ENUMERATE_SUPPORT</DefineConstants>
    <DefineConstants Condition="$(TargetFramework.StartsWith('net4'))">$(DefineConstants);ALLOW_PARTIALLY_TRUSTED_CALLERS;ENABLE_HTTP_HANDLER</DefineConstants>
+    <DefineConstants Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">$(DefineConstants);NET5</DefineConstants>


What happens in the future when NetCoreAppCurrent is > 5? Do we need to add netcoreapp5.0 to the TargetFrameworks list explicitly?

Do we need to add netcoreapp5.0 to the TargetFrameworks list explicitly?

No

What happens in the future when NetCoreAppCurrent is > 5?

NetCoreAppCurrent will have the right next version of netcoreapp. so in 6.0 will have it as 6.0. in short this should work moving forward. I chose the defined name as NET5 to be clear when we have introduced this change when looking at the code.

I think @ericstj raised a similar concern. The scenario I am worried about is that this NuGet package is distributed as an OOB package. Assume in 2021 we ship a new version that targets netcoreapp6 and people keeping their netcoreapp5.0 app upgrade this package only. I assume that NuGet will resolve the reference to the netstandard implementation because the updated version no longer targets netcoreapp5.0. The netstandard implementation wouldn't have default W3C id so the upgrade will will cause them to lose the change. I'm no expert in NuGet resolution so if I am misunderstanding the mechanics here just let me know : )

Yes, you are right. I am talking to @ericstj if we need to include netcoreapp targeting to the package to mitigate this issue.
CC @joperezr

You've got it right. We need to include the net5.0 build in the package. We might also remove the netstandard2.0 build from this project. I don't see anywhere it would actually be used. We don't ship it in the package and it's not used in any other scenario.

ok, thanks @ericstj. I have done that. please have a quick look at the last commit just to confirm the change.

@ericstj could you please have a quick look at last commit and let me know? I want to ensure we are good to go.

Actually I am seeing tons of failures like

##[error].dotnet/sdk/5.0.100-preview.6.20310.4/Microsoft.Common.CurrentVersion.targets(2084,5): error MSB3245: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not resolve this reference. Could not locate the assembly "System.Memory". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. /__w/1/s/.dotnet/sdk/5.0.100-preview.6.20310.4/Microsoft.Common.CurrentVersion.targets(2084,5): error MSB3245: Could not resolve this reference. Could not locate the assembly "System.Memory". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [/__w/1/s/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj]

https://dev.azure.com/dnceng/public/_build/results?buildId=693198&view=logs&j=5ea74393-5ab8-5088-11c3-b14699e9d5af&t=c9c7c300-3b25-5026-7590-be769f231fb8&l=5751

Yeah that is because we actually need to keep ns2.0 configuration as it is used for Build From Source. In Build from source, apart from building the regular vertical, we also build the netstandard 2.0 vertical: https://github.com/dotnet/runtime/blob/master/src/libraries/Directory.Build.props#L288 and for that we were building the ns2.0 config on this project, but now the one that applies is ns1.3 which you can't build in build from source as we don't have the full ns1.3 targeting pack.

I returned the netstandard 2.0 back for now.

...libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

ericstj · 2020-06-17T21:16:55Z

What happens when a .NET5.0 application installs the DiagnosticSource from .NET5.1 or .NET5.0 servicing?

The net5.0 build is not in the package:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

Lines 8 to 9 in 90af505

    
           <ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage> 
        
           <ExcludeCurrentFullFrameworkFromPackage>true</ExcludeCurrentFullFrameworkFromPackage>

So the package would downgrade functionality. We may need to include the net5.0 build in the package.

When removing it, caused a CI source build failure

ericstj · 2020-06-19T15:55:03Z

Removing the netstandard2.0 configuration failed in the source-build leg. That's because during source build we do a pass of "best" src libraries for both NetCoreAppCurrrent, netstandard2.1, and netstandard2.0. We do this because in source build we don't have all reference assemblies (EG: past netcoreapps, netstandard1.x) but we do have netstandard2.x references. We try to build as much as we can so that upstream package consumers get what they need out of our packages.

The netstandard2.0 config here was building in source-build and preventing netstandard1.3 from being "best". Once we removed it that made ns1.3 as best, and the build fails due to source-build being unable to satisfy the reference assemblies for this target.

Dotnet-GitSync-Bot added the area-System.Diagnostics.Tracing label Jun 10, 2020

tarekgh requested a review from noahfalk June 10, 2020 02:00

tarekgh added the new-api-needs-documentation label Jun 10, 2020

stephentoub added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 10, 2020

Change Activity Default IdFormat to W3C

c76568f

tarekgh force-pushed the SetActivityDefaultIdFormatToW3C branch from de3f240 to c76568f Compare June 10, 2020 02:44

davidfowl reviewed Jun 10, 2020

View reviewed changes

...raries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LocalAppContextSwitches.cs Outdated Show resolved Hide resolved

Remove the prefix from the switch and fix a test

5d4e02e

noahfalk reviewed Jun 11, 2020

View reviewed changes

noahfalk requested review from shirhatti and richlander June 11, 2020 02:42

address some of the feedback

b2a6953

shirhatti mentioned this pull request Jun 15, 2020

Change default ActivityIdFormat to W3C dotnet/docs#18953

Closed

tarekgh added 2 commits June 16, 2020 16:52

Scope the breaking change to .NET 5

78199db

Merge branch 'master' of https://github.com/dotnet/runtime into SetAc…

7ad5cfa

…tivityDefaultIdFormatToW3C

Fix running the test on NetFX

3f53000

noahfalk approved these changes Jun 17, 2020

View reviewed changes

Changed the define NET5 to W3C_DEFAULT_ID_FORMAT

90af505

jaredpar mentioned this pull request Jun 17, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

tarekgh added 2 commits June 17, 2020 19:39

Add net5 and remove netstandard2.0 configuration in the package

60751c1

Return back netstandard 2.0 targetting

2db49d2

When removing it, caused a CI source build failure

tarekgh merged commit c5edc00 into dotnet:master Jun 19, 2020

tarekgh deleted the SetActivityDefaultIdFormatToW3C branch June 22, 2020 16:11

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Activity Default IdFormat to W3C #37686

Change Activity Default IdFormat to W3C #37686

tarekgh commented Jun 10, 2020 •

edited

Loading

ghost commented Jun 10, 2020

tarekgh commented Jun 10, 2020

tarekgh commented Jun 10, 2020

tarekgh commented Jun 11, 2020 •

edited

Loading

noahfalk left a comment

shirhatti commented Jun 15, 2020 •

edited

Loading

tarekgh commented Jun 15, 2020

tarekgh commented Jun 15, 2020

tarekgh commented Jun 17, 2020 •

edited

Loading

noahfalk left a comment

noahfalk Jun 17, 2020

tarekgh Jun 17, 2020

noahfalk Jun 17, 2020

tarekgh Jun 17, 2020

ericstj Jun 17, 2020

tarekgh Jun 18, 2020

tarekgh Jun 18, 2020

tarekgh Jun 18, 2020

joperezr Jun 18, 2020

tarekgh Jun 18, 2020

ericstj commented Jun 17, 2020

ericstj commented Jun 19, 2020

Change Activity Default IdFormat to W3C #37686

Change Activity Default IdFormat to W3C #37686

Conversation

tarekgh commented Jun 10, 2020 • edited Loading

ghost commented Jun 10, 2020

tarekgh commented Jun 10, 2020

tarekgh commented Jun 10, 2020

tarekgh commented Jun 11, 2020 • edited Loading

noahfalk left a comment

Choose a reason for hiding this comment

shirhatti commented Jun 15, 2020 • edited Loading

tarekgh commented Jun 15, 2020

tarekgh commented Jun 15, 2020

tarekgh commented Jun 17, 2020 • edited Loading

noahfalk left a 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

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

ericstj commented Jun 17, 2020

ericstj commented Jun 19, 2020

tarekgh commented Jun 10, 2020 •

edited

Loading

tarekgh commented Jun 11, 2020 •

edited

Loading

shirhatti commented Jun 15, 2020 •

edited

Loading

tarekgh commented Jun 17, 2020 •

edited

Loading