Figure out a way to trace through AsyncIterator

[xirzec]

Today methods that use the AsyncIterator pattern return a PagedAsyncIterableIterator. Under the covers, these methods call into a generator that returns the shape AsyncIterableIterator that eventually delegate to a method that makes a single request.

It's easy to create tracing Spans for the underlying method that performs a request, but it's less clear about what Spans should be created for the iterator itself. Since an iterator may never finish iterating its entire collection, starting a Span at the beginning of iteration might never finish.

[xirzec]

keyvault-keys and keyvault-secrets currently instrument list* operations, but the parent span ends as soon as the iterator is created, which isn't great, since then child spans can be longer.

It looks like appconfiguration maybe does the same.

[xirzec]

Same in keyvault libs

[maorleger]

Lots has changed since this issue was made! Going to list my thoughts and revisit in the new year:

keyvault-keys and keyvault-secrets currently instrument list* operations, but the parent span ends as soon as the iterator is created, which isn't great, since then child spans can be longer.

It looks like appconfiguration maybe does the same.

I also noticed that we're just tracing the creation of the iterator which doesn't really trace anything interesting so at the time I just forged ahead with tracing the page fetching operation instead.

azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/index.ts

Lines 922 to 926 in f31aa42

	const currentSetResponse = await withTrace(
	"listPropertiesOfKeyVersionsPage",
	optionsComplete,
	async (updatedOptions) => this.client.getKeyVersions(this.vaultUrl, name, updatedOptions)
	);

azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/index.ts

Lines 934 to 939 in f31aa42

	const currentSetResponse = await withTrace(
	"listPropertiesOfKeyVersionsPage",
	options || {},
	async (updatedOptions) =>
	this.client.getKeyVersions(continuationState.continuationToken!, name, updatedOptions)
	);

The downside of this approach is:

1. that can be a little confusing if you do something like

for await (const thingy of client.listThings) { ... }

And then end up with N spans (depending on number of pages and how many you consume) at the convenience layer called listThingsPage

2. It leaks the implementation detail that for await is an abstraction over byPage

But it is nice to trace over it, otherwise you'll just end up with some rogue http spans anyway when tracingPolicy runs. And as a consumer you can wrap the iteration in a logical parent span

Next step for me would be to see what this looks like in Jaeger when you have 100 secrets or something

Going to revisit this and see if this approach makes sense for general guidance...

[maorleger]

By the way, I demo'd this to @lmolkova and she was happy with the direction here. She had a few additional improvements like adding the continuation token as a span attribute.

If we are happy with the approach I listed above - @xirzec do you like this approach or not really? Assuming you are good with this - what do we need to close out this issue? Just guidance? Issues open for all packages? Update code for all packages?

[xirzec]

This seems like the most sensible thing we can do, even if the method itself isn't public it's pretty clear what is going on. We could establish a convention for how we name this thing to make it consistent (e.g. always take the list method name and suffix with Page)

[maorleger]

I have #20399 demonstrating this approach and #20411 tracking adding all this info to the guidelines in the next semester so I think it's safe to close this. Let me know if you disagree and we can reopen it.