Validate that library scope is present when using ForwardClientCalls #28469
Conversation
JoshLove-msft
requested review from
christothes,
annelo-msft and
KrzysztofCwalina
as code owners
ghost
added
the
|
API change check for API changes are not detected in this pull request for |
sdk/core/Azure.Core.TestFramework/src/Instrumentation/DiagnosticScopeValidatingInterceptor.cs
Outdated
Show resolved
Hide resolved
| @@ -209,9 +209,14 @@ internal static async ValueTask<T> ValidateDiagnosticScope<T>(Func<ValueTask<(T | |||
| } | |||
| else | |||
| { | |||
| if (!diagnosticListener.Scopes.Any()) | |||
| // If ForwardsClientCallsAttribute is being used on the method, we don't know what the name of the scope should be because there could be many | |||
There was a problem hiding this comment.
Why are Azure.Core scopes created? Scopes are supposed to correspond to customer calls of public methods. Or are these the inner HTTP scopes?
There was a problem hiding this comment.
Maybe we could defined "The Azure.Core scope?"
There was a problem hiding this comment.
Per #27828 (comment), perhaps we could call the "nested HTTP scopes"?
There was a problem hiding this comment.
Why are Azure.Core scopes created? Scopes are supposed to correspond to customer calls of public methods. Or are these the inner HTTP scopes?
Azure.Core creates scopes for outgoing requests - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs#L57
There was a problem hiding this comment.
, perhaps we could call the "nested HTTP scopes"?
I'm not sure what this comment is referring to.
There was a problem hiding this comment.
there should be some scope created other than the Azure.Core scope.
It's a nit, but in the distributed tracing guidelines, I've heard these referred to as the "nested HTTP/REST spans", and never as Azure.Core scopes, see https://azure.github.io/azure-sdk/general_implementation.html#distributed-tracing
There was a problem hiding this comment.
Ah I see. That is a fair point.
…28469) * Validate that library scope is present when using ForwardClientCalls * Fix comment * Management tests * fix * PR fb * space * Remove duplicate braces
…zure#28469) * Validate that library scope is present when using ForwardClientCalls * Fix comment * Management tests * fix * PR fb * space * Remove duplicate braces
Fixes #11966