Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ASP.NET Core] Do not reset baggage on request stop event #4274

Merged
merged 11 commits into from
May 5, 2023
Merged

[ASP.NET Core] Do not reset baggage on request stop event #4274

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7c098e7
[ASP.NET Core] Do not reset baggage on requeststop
Mar 6, 2023
00ec5b7
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
vishweshbankwar Mar 10, 2023
c571d75
add changelog
Mar 10, 2023
01b1146
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
Apr 12, 2023
5879da9
newline
Apr 12, 2023
4f4cccb
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
Apr 18, 2023
db4941b
Update src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
vishweshbankwar Apr 20, 2023
063645a
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
vishweshbankwar Apr 24, 2023
a98f37d
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
cijothomas Apr 27, 2023
3616c23
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
cijothomas May 5, 2023
0154e55
Merge branch 'main' into vibankwa/remove-baggage-clearance-aspnetcore
utpilla May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

* Baggage will not be reset during request stop event. This will allow
processors to access it during OnEnd call.
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
([#4274](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4274))

* Improve perf by avoiding boxing of common status codes values.
([#4360](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4360),
[#4363](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4363))
Expand Down
6 changes: 0 additions & 6 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,12 +286,6 @@ public void OnStopActivity(Activity activity, object payload)
// the one created by the instrumentation.
// And retrieve it here, and set it to Current.
}

var textMapPropagator = Propagators.DefaultTextMapPropagator;
if (textMapPropagator is not TraceContextPropagator)
{
Baggage.Current = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be done at SDK side instead? Could you run stress tests to confirm there are no leaks if we do not remove it at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did run stress test. Did not see any leaks.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be done at SDK side instead?

Are you referring to OnEnd in SDK? I have not looked in to that but just thinking it will impact all activities as we need to first check the activity ending is ASP.NET Core one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baggage should be independent of any activity, so why we need to check which activity is being stopped?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for ASP.NET Core activity. For other one's we do not do anything right now as well.

}
}

public void OnMvcBeforeAction(Activity activity, object payload)
Expand Down
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ public async Task ExtractContextIrrespectiveOfTheFilterApplied()
}

[Fact]
public async Task BaggageClearedWhenActivityStopped()
public async Task BaggageIsNotClearedWhenActivityStopped()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocking comment... but if the instrumentation had never had this behavior, then we likely would never had this test in the first place. Perhaps we just remove this test completely?

{
int? baggageCountAfterStart = null;
int? baggageCountAfterStop = null;
Expand Down Expand Up @@ -532,7 +532,7 @@ void ConfigureTestServices(IServiceCollection services)
Assert.NotNull(baggageCountAfterStart);
Assert.Equal(2, baggageCountAfterStart);
Assert.NotNull(baggageCountAfterStop);
Assert.Equal(0, baggageCountAfterStop);
Assert.Equal(2, baggageCountAfterStop);
}

[Theory]
Expand Down