Skip to content

Preserve client order of activity baggage items #26302

New issue

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

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

Already on GitHub? # to your account

Merged

Conversation

AndreyTretyak
Copy link
Contributor

@AndreyTretyak AndreyTretyak commented Sep 24, 2020

AddBaggage in reverse order to be the same as on the client

Addresses #26303

AddBaggage adds items at the beginning of the list, so we need to add them in reverse to keep the same order as the client
An order could be important if baggage has two items with the same key (that is allowed by the contract)
@ghost ghost added the area-hosting label Sep 24, 2020
@AndreyTretyak AndreyTretyak marked this pull request as ready for review September 25, 2020 13:22
@Tratcher
Copy link
Member

Test failure:
https://dev.azure.com/dnceng/public/_build/results?buildId=830373&view=ms.vss-test-web.build-test-results-tab&runId=26468712&resultId=105063&paneView=debug
Microsoft.AspNetCore.Hosting.Tests.HostingApplicationDiagnosticsTests.ActivityBaggagePreservesItemsOrder

Assert.Equal() Failure
Expected: KeyValuePair`2[] [[Key1, value1], [Key2, value2], [Key1, value3]]
Actual:   <<get_Baggage>g__Iterate|71_0>d [[Key1, value3], [Key2, value2], [Key1, value1]]

@Tratcher Tratcher self-assigned this Sep 25, 2020
@AndreyTretyak
Copy link
Contributor Author

AndreyTretyak commented Sep 25, 2020

Sorry, I forgot to mention.
There is a bug now in Activity for net5.0 rc1 dotnet/runtime#42659
After it would be fixed we would get the previous behavior of the Activity and UT should start working :)

@Tratcher
Copy link
Member

@shirhatti @davidfowl @Pilchie is this a 5.0 candidate? It looks like the runtime PR was backported to 5.0.

@davidfowl
Copy link
Member

This is broken in 3.x as well no?

@AndreyTretyak
Copy link
Contributor Author

AndreyTretyak commented Sep 25, 2020

So HostingApplicationDiagnostics.cs behaivour was changing Activity.Baggage order in version 3.x, but in net5.0 preview version of the Activity there was a breaking change and in combination they worked properly. After Activity bug will be fixed it would be broken again (if we won't make the change).

@AndreyTretyak
Copy link
Contributor Author

AndreyTretyak commented Sep 26, 2020

As far as I understand you are quite close to the 5.0 release, but in case if for some reason you're still considering including this fix I'll add some details why it's more important before net6.0.

Currently, the only way of changing Activity.Baggage is to call AddBaggage and if you call it twice for the same key it would look like the value was updated, but you're actually getting two values with the same key and after transfer GetBaggageItem will start returning the old value. So this case might be more common then it looks.

net6.0 could have an API for updating Baggage(dotnet/runtime#42706) and it might reduce the number of cases with the duplicated keys.

@Tratcher Tratcher merged commit 3892b4e into dotnet:master Sep 28, 2020
@Tratcher Tratcher added this to the 6.0.0-alpha1 milestone Sep 28, 2020
@Tratcher
Copy link
Member

I don't see folks pushing to get this into 5.0 so it's been merged into master for 6.0.

@AndreyTretyak
Copy link
Contributor Author

Okay, thanks, do I need to do something with the bug attached or it's up to you?

@shirhatti shirhatti added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Sep 28, 2020
@shirhatti
Copy link
Contributor

We should create a breaking change announcement for this behavior change

@AndreyTretyak AndreyTretyak deleted the andreyt/change-activity-baggage-order branch September 28, 2020 22:18
wtgodbe pushed a commit that referenced this pull request Oct 5, 2020
Co-authored-by: Andrey Tretyak <andreyt@microsoft.com>
wtgodbe pushed a commit that referenced this pull request Oct 13, 2020
#26398)

* Preserve client order of activity baggage items (#26302)

* Add missing using

Co-authored-by: Andrey Tretyak <andreyt@microsoft.com>
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-hosting Includes Hosting breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants