Skip to content

Commit 5920767

Browse files
Remove unnecessary distributed tracing scopes from paging methods (#54006)
* Initial plan * Add logic to skip distributed tracing for paging methods and add tests Co-authored-by: JoshLove-msft <[email protected]> * Regenerate BasicTypeSpec test project with updated distributed tracing visitor Co-authored-by: JoshLove-msft <[email protected]> * Refactor ShouldSkip to take ScmMethodProvider and check for paging methods - Updated ShouldSkip method to accept ScmMethodProvider instead of TypeProvider - Added ShouldSkipType method for constructor visitor to check TypeProvider - Added IsFrameworkType check in IsPagingMethod to avoid exceptions on non-framework types - This ensures paging methods are skipped for all method kinds, not just protocol methods Co-authored-by: JoshLove-msft <[email protected]> * Rename ShouldSkip to ShouldSkipMethod and remove redundant type check - Renamed ShouldSkip method to ShouldSkipMethod for clarity - Removed redundant typeProvider check from ShouldSkipMethod - Updated VisitMethod to explicitly call both ShouldSkipType and ShouldSkipMethod - ShouldSkipMethod now only contains the paging method check logic Co-authored-by: JoshLove-msft <[email protected]> * Add test coverage for convenience method paging instrumentation - Extended TestSkipsInstrumentationForPagingMethods to test both Protocol and Convenience method kinds - Added 2 new test cases for convenience methods (async and sync) - Now testing 4 scenarios: Protocol async/sync and Convenience async/sync - All tests verify that paging methods do not have DiagnosticScope instrumentation Co-authored-by: JoshLove-msft <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JoshLove-msft <[email protected]>
1 parent c685ebd commit 5920767

File tree

3 files changed

+91
-135
lines changed

3 files changed

+91
-135
lines changed

eng/packages/http-client-csharp/generator/Azure.Generator/src/Visitors/DistributedTracingVisitor.cs

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
using Azure;
45
using Azure.Core.Pipeline;
56
using Microsoft.TypeSpec.Generator.ClientModel;
67
using Microsoft.TypeSpec.Generator.ClientModel.Providers;
@@ -49,7 +50,7 @@ internal class DistributedTracingVisitor : ScmLibraryVisitor
4950

5051
protected override ConstructorProvider? VisitConstructor(ConstructorProvider constructor)
5152
{
52-
if (ShouldSkip(constructor.EnclosingType))
53+
if (ShouldSkipType(constructor.EnclosingType))
5354
{
5455
return base.VisitConstructor(constructor);
5556
}
@@ -102,7 +103,7 @@ internal class DistributedTracingVisitor : ScmLibraryVisitor
102103

103104
protected override ScmMethodProvider? VisitMethod(ScmMethodProvider method)
104105
{
105-
if (ShouldSkip(method.EnclosingType))
106+
if (ShouldSkipType(method.EnclosingType) || ShouldSkipMethod(method))
106107
{
107108
return base.VisitMethod(method);
108109
}
@@ -266,13 +267,20 @@ private static bool TryUpdateSubClientFactoryMethodReturnStatement(
266267
return true;
267268
}
268269

269-
private static bool ShouldSkip(TypeProvider typeProvider)
270+
private static bool ShouldSkipType(TypeProvider typeProvider)
270271
{
271272
return typeProvider is not ClientProvider ||
272273
!typeProvider.CanonicalView.Properties
273274
.Any(p => p.Name == ClientDiagnosticsPropertyName || p.OriginalName?.Equals(ClientDiagnosticsPropertyName) == true);
274275
}
275276

277+
private static bool ShouldSkipMethod(ScmMethodProvider method)
278+
{
279+
// Skip instrumentation for methods returning paging collection types
280+
// as they have built-in instrumentation
281+
return IsPagingMethod(method);
282+
}
283+
276284
private static bool IsSubClientFactoryMethod(ScmMethodProvider method)
277285
{
278286
ClientProvider clientProvider = (ClientProvider)method.EnclosingType;
@@ -281,5 +289,18 @@ private static bool IsSubClientFactoryMethod(ScmMethodProvider method)
281289
return methodReturnType != null &&
282290
clientProvider.SubClients.Any(subClient => methodReturnType.Equals(subClient.Type));
283291
}
292+
293+
private static bool IsPagingMethod(ScmMethodProvider method)
294+
{
295+
var returnType = method.Signature.ReturnType;
296+
if (returnType == null || !returnType.IsFrameworkType)
297+
{
298+
return false;
299+
}
300+
301+
// Check if the return type is Pageable<T> or AsyncPageable<T>
302+
return returnType.FrameworkType == typeof(Pageable<>) ||
303+
returnType.FrameworkType == typeof(AsyncPageable<>);
304+
}
284305
}
285306
}

eng/packages/http-client-csharp/generator/Azure.Generator/test/Visitors/DistributedTracingVisitorTests.cs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
using System;
5+
using Azure;
46
using Azure.Core.Pipeline;
57
using Azure.Generator.Tests.Common;
68
using Azure.Generator.Tests.TestHelpers;
@@ -164,6 +166,59 @@ public void TestUpdatesProtocolMethods(bool isProtocolMethod)
164166
Assert.AreEqual(Helpers.GetExpectedFromFile(isProtocolMethod.ToString()), result);
165167
}
166168

169+
[TestCase(true, ScmMethodKind.Protocol)]
170+
[TestCase(false, ScmMethodKind.Protocol)]
171+
[TestCase(true, ScmMethodKind.Convenience)]
172+
[TestCase(false, ScmMethodKind.Convenience)]
173+
public void TestSkipsInstrumentationForPagingMethods(bool isAsync, ScmMethodKind methodKind)
174+
{
175+
var visitor = new TestDistributedTracingVisitor();
176+
177+
// load the input
178+
List<InputMethodParameter> parameters =
179+
[
180+
InputFactory.MethodParameter(
181+
"context",
182+
InputPrimitiveType.String)
183+
];
184+
var basicOperation = InputFactory.Operation(
185+
"listItems",
186+
parameters: parameters);
187+
var basicServiceMethod = InputFactory.BasicServiceMethod("listItems", basicOperation, parameters: parameters);
188+
var inputClient = InputFactory.Client("TestClient", methods: [basicServiceMethod]);
189+
MockHelpers.LoadMockGenerator(clients: () => [inputClient]);
190+
// create the client provider
191+
var clientProvider = AzureClientGenerator.Instance.TypeFactory.CreateClient(inputClient);
192+
Assert.IsNotNull(clientProvider);
193+
194+
// create a paging method to test the visitor
195+
var pagingReturnType = isAsync
196+
? new CSharpType(typeof(AsyncPageable<>), typeof(BinaryData))
197+
: new CSharpType(typeof(Pageable<>), typeof(BinaryData));
198+
199+
var methodSignature = new MethodSignature(
200+
isAsync ? "ListItemsAsync" : "ListItems",
201+
null,
202+
MethodSignatureModifiers.Public | MethodSignatureModifiers.Virtual,
203+
pagingReturnType,
204+
$"The pageable response returned from the service.",
205+
[new ParameterProvider("context", $"The request context", AzureClientGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType)]);
206+
var bodyStatements = Return(New.Instance(pagingReturnType));
207+
var method = new ScmMethodProvider(methodSignature, bodyStatements, clientProvider!, methodKind);
208+
209+
var updatedMethod = visitor.InvokeVisitMethod(method!);
210+
Assert.IsNotNull(updatedMethod?.BodyStatements);
211+
212+
var result = updatedMethod!.BodyStatements!.ToDisplayString();
213+
// Verify that the method body does NOT contain DiagnosticScope instrumentation
214+
Assert.IsFalse(result.Contains("DiagnosticScope"),
215+
$"Paging method should not have DiagnosticScope instrumentation. Method: {(isAsync ? "AsyncPageable" : "Pageable")}, Kind: {methodKind}");
216+
Assert.IsFalse(result.Contains("scope.Start()"),
217+
$"Paging method should not have scope.Start() call. Method: {(isAsync ? "AsyncPageable" : "Pageable")}, Kind: {methodKind}");
218+
Assert.IsFalse(result.Contains("scope.Failed"),
219+
$"Paging method should not have scope.Failed() call. Method: {(isAsync ? "AsyncPageable" : "Pageable")}, Kind: {methodKind}");
220+
}
221+
167222
private static IEnumerable<TestCaseData> TestUpdatesConstructorsTestCases
168223
{
169224
get

eng/packages/http-client-csharp/generator/TestProjects/Local/Basic-TypeSpec/src/Generated/BasicTypeSpecClient.cs

Lines changed: 12 additions & 132 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)