-
Notifications
You must be signed in to change notification settings - Fork 481
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.
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
Gateway Trace: Fixes a bug where the ActivityId is being set to Guid.Empty #1720
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors) Description"
Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.
@@ -162,10 +162,6 @@ internal static INameValueCollection ExtractResponseHeaders(HttpResponseMessage | |||
HttpResponseMessage responseMessage, | |||
IClientSideRequestStatistics requestStatistics) | |||
{ | |||
// ensure there is no local ActivityId, since in Gateway mode ActivityId | |||
// should always come from message headers | |||
Trace.CorrelationManager.ActivityId = Guid.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a UT that checks that the ActivityId is not empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a dry run with our gates to see which paths will fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug assert that I added verifies it for all tests.
Looks good to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the comments.
…Empty (#1720) * Fixed tracing activity id * Reverted unnecessary changes * Always uses the local activity id
Closing due to in-activity, pease feel free to re-open. |
Pull Request Template
Description
This fixes a bug where Trace.CorrelationManager.ActivityId was getting set to Guid.Empty instead of the response Activity Id. This causes any traces to have Guid.Empty instead of the correct value.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber