Enable SDK to create package for auto-instrumentation #2365

rajkumar-rangaraj · 2021-08-13T06:16:39Z

To upgrade Application Insights SDK for .NET Core instrumentation following are the requirements and this PR propose the changes for it.

Downgrade System.Diagnostics.DiagnosticSource package reference to 4.7.0 from 5.0.0.
Rename event source names to support side-by-side load from both Nuget and auto-instrumentation AI SDK.
Build the package from AI SDK’s official release pipeline

Changes

(Please provide a brief description of the changes here.)

Downgraded DiagnosticSource
Added conditional compilation field REDFIELD which is used as preprocessor to decide the event source name
Enabled redfield compilation switch to build and generate package for auto-instrumentation

…ights-dotnet into rajrang/redfield

…cationInsights-dotnet into rajrang/redfield

Directory.Build.props

.props/_GlobalStaticVersion.props

...nInsights/Extensibility/Implementation/Tracing/DiagnosticsModule/DiagnosticsEventListener.cs

BASE/src/Microsoft.ApplicationInsights/Microsoft.ApplicationInsights.csproj

reyang

LGTM with some suggestions.

...ghts/Extensibility/Implementation/Tracing/FileDiagnosticsModule/TraceSourceForEventSource.cs

WEB/Src/DependencyCollector/DependencyCollector/DependencyTrackingTelemetryModule.cs

...rosoft.ApplicationInsights.Tests/Extensibility/Implementation/Tracing/CoreEventSourceTest.cs

cijothomas · 2021-08-14T05:17:34Z

Note: Won't be able to support https://github.com/microsoft/ApplicationInsights-dotnet/pull/2185/files with this change

Directory.Build.props

rajkumar-rangaraj · 2021-08-16T19:19:35Z

Note: Won't be able to support https://github.com/microsoft/ApplicationInsights-dotnet/pull/2185/files with this change

We added a redfied sanity check in GitHub actions. This ensures builds are not broken with new changes.

This is a temporary workaround until we resolve the DiagnosticSource versioning issue. We will remove these changes once the DiagnosticSource issue is resolved.

.github/workflows/redfield-sanity-check.yml

cijothomas · 2021-08-18T03:02:10Z

.github/workflows/redfield-sanity-check.yml

+      run: dotnet restore ./BASE/Microsoft.ApplicationInsights.sln
+
+    - name: Build
+      run: dotnet build -p:Redfield=True ./BASE/Microsoft.ApplicationInsights.sln --configuration Release --no-restore


only building Base sdk?

Running our full test suite here will fail.
We have too many flaky tests and Github Actions does not have a mechanism for retries.
(I'm investigating this in #2345)
If we want to run more test projects here, we must filter those tests.

cijothomas · 2021-08-18T03:06:53Z

BASE/src/Microsoft.ApplicationInsights/ActivityExtensions.cs

@@ -52,7 +52,11 @@ private static bool Initialize()
        {
            try
            {
+#if REDFIELD


Can you open an issue to completely remove this class. it should not be required.

Investigated a little and found methods from this class is called in several places.
For example Tryrun is called in OperationCorrelationTelemetryInitializer.cs.

Did I understand your comment incorrectly?

yea all the places where this ActivityExtensions is used can be modified to directly use Activity API, instead of doing it via this class.

BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/RichPayloadEventSource.cs

cijothomas · 2021-08-18T03:14:07Z

WEB/Src/DependencyCollector/DependencyCollector/Implementation/AppMapCorrelationEventSource.cs

@@ -7,7 +7,11 @@
    /// <summary>
    /// ETW EventSource tracing class.
    /// </summary>
+#if REDFIELD


for a separate PR , this file seems doing nothing and could be removed!

cijothomas · 2021-08-18T03:17:17Z

Note: Won't be able to support https://github.com/microsoft/ApplicationInsights-dotnet/pull/2185/files with this change

We added a redfied sanity check in GitHub actions. This ensures builds are not broken with new changes.

This is a temporary workaround until we resolve the DiagnosticSource versioning issue. We will remove these changes once the DiagnosticSource issue is resolved.

I think we can support PRs like the above, by doing it inside IF !REDFIELD check, as we'll have 5.0.0 of DS when not-redfield.

rajkumar-rangaraj added 5 commits August 12, 2021 23:12

Create downgraded DiagnosticSource version for Redfield

1682ccd

Merge branch 'develop' into rajrang/redfield

3f7cb23

Merge branch 'develop' of https://github.com/microsoft/ApplicationIns…

f34357a

…ights-dotnet into rajrang/redfield

Remove Settings.Designer.cs, created by mistake

e4b8772

Merge branch 'rajrang/redfield' of https://github.com/microsoft/Appli…

c6229af

…cationInsights-dotnet into rajrang/redfield

TimothyMothra reviewed Aug 13, 2021

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

rajkumar-rangaraj added 4 commits August 13, 2021 11:19

Fix tests

e5fff6a

Fix tests

9048c49

Fix tests

9544b3d

Remove hard-coded Redfield property

f115ab1

rajkumar-rangaraj changed the title ~~[Draft] Enable SDK to be aware of auto-instrumentation~~ Enable SDK to create package for auto-instrumentation Aug 13, 2021

reyang reviewed Aug 13, 2021

View reviewed changes

.props/_GlobalStaticVersion.props Outdated Show resolved Hide resolved

reyang reviewed Aug 13, 2021

View reviewed changes

...nInsights/Extensibility/Implementation/Tracing/DiagnosticsModule/DiagnosticsEventListener.cs Outdated Show resolved Hide resolved

reyang reviewed Aug 13, 2021

View reviewed changes

BASE/src/Microsoft.ApplicationInsights/Microsoft.ApplicationInsights.csproj Outdated Show resolved Hide resolved

reyang approved these changes Aug 13, 2021

View reviewed changes

vishweshbankwar reviewed Aug 13, 2021

View reviewed changes

...ghts/Extensibility/Implementation/Tracing/FileDiagnosticsModule/TraceSourceForEventSource.cs Show resolved Hide resolved

PR Comments

d1581be

rajkumar-rangaraj mentioned this pull request Aug 14, 2021

[WIP - Draft] Downgrade Diagnostic Source Test #2299

Closed

xiang17 reviewed Aug 14, 2021

View reviewed changes

WEB/Src/DependencyCollector/DependencyCollector/DependencyTrackingTelemetryModule.cs Show resolved Hide resolved

...rosoft.ApplicationInsights.Tests/Extensibility/Implementation/Tracing/CoreEventSourceTest.cs Show resolved Hide resolved

cijothomas reviewed Aug 14, 2021

View reviewed changes

Directory.Build.props Show resolved Hide resolved

rajkumar-rangaraj added 2 commits August 16, 2021 11:28

Resolve conflict and add github actions

f2c7969

Rename yml file

2974011

TimothyMothra approved these changes Aug 16, 2021

View reviewed changes

TimothyMothra reviewed Aug 16, 2021

View reviewed changes

.github/workflows/redfield-sanity-check.yml Show resolved Hide resolved

cijothomas reviewed Aug 18, 2021

View reviewed changes

BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/RichPayloadEventSource.cs Show resolved Hide resolved

cijothomas reviewed Aug 18, 2021

View reviewed changes

cijothomas approved these changes Aug 18, 2021

View reviewed changes

rajkumar-rangaraj merged commit 0c25b41 into develop Aug 18, 2021

rajkumar-rangaraj deleted the rajrang/redfield branch August 18, 2021 16:52

rajkumar-rangaraj mentioned this pull request Aug 18, 2021

Update ChangeLog to include auto-instrumentation changes #2370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable SDK to create package for auto-instrumentation #2365

Enable SDK to create package for auto-instrumentation #2365

rajkumar-rangaraj commented Aug 13, 2021 •

edited

Loading

reyang left a comment

cijothomas commented Aug 14, 2021

rajkumar-rangaraj commented Aug 16, 2021

cijothomas Aug 18, 2021

TimothyMothra Aug 18, 2021

cijothomas Aug 18, 2021

rajkumar-rangaraj Aug 18, 2021

cijothomas Aug 18, 2021

cijothomas Aug 18, 2021

cijothomas commented Aug 18, 2021

Enable SDK to create package for auto-instrumentation #2365

Enable SDK to create package for auto-instrumentation #2365

Conversation

rajkumar-rangaraj commented Aug 13, 2021 • edited Loading

Changes

reyang left a comment

Choose a reason for hiding this comment

cijothomas commented Aug 14, 2021

rajkumar-rangaraj commented Aug 16, 2021

cijothomas Aug 18, 2021

Choose a reason for hiding this comment

TimothyMothra Aug 18, 2021

Choose a reason for hiding this comment

cijothomas Aug 18, 2021

Choose a reason for hiding this comment

rajkumar-rangaraj Aug 18, 2021

Choose a reason for hiding this comment

cijothomas Aug 18, 2021

Choose a reason for hiding this comment

cijothomas Aug 18, 2021

Choose a reason for hiding this comment

cijothomas commented Aug 18, 2021

rajkumar-rangaraj commented Aug 13, 2021 •

edited

Loading