feat: make custom attributes function async

[cmackenthun]

trafficstars

Which problem is this PR solving?

In the applyCustomAttributes function, we are provided the request and response objects which each have async functions that can be used to read the response body and request body and use those to add different attributes to the spans. The problem is that this function call is not awaited in the fetch instrumentation, so there are times where the span is ended before the attributes can be added and are therefore missed.

This change will properly await the custom attributes function and will not affect synchronous usages of the function. I have also moved up the calculation of the span end time to make it more accurate to the length of time that the request took and not include the time that it took to run the custom attributes function

Fixes # (issue)

Short description of the changes

• Typing applyCustomAttributes to accept an async function
• Awaiting applyCustomAttributes so that it can apply attributes without worry of the span being ended
• Moving calculation of end time up further in callstack to make it more accurate to the request length of time

Type of change

• [x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

1. I set up a applyCustomAttributes function that includes a 10 second timeout before returning
2. I ran the fetch instrumentation without these changes and got the Can not execute the operation on ended Span error in the console
3. I added my changes and used yalc to add the package to my setup
4. I ran the application again and saw the attributes being added after my ten second timeout without error

Checklist:

• [x] Followed the style guidelines of this project
• [x] Unit tests have been added
• [x] Documentation has been updated

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: cmackenthun (c0589d7788bd630b89e7618e5a50c3f171e054d4, 8f088203d50c20f89079b9a7cdf09630065b43cf, c0e5ef4a295608fd554784bf04e47beb62f0c4a1, 7e6ccba03796a38ce31311b2a4d01f6997a5b86e, 846fa41f9e6e3c78a774d9d2d649a689baa3eca4, 9c8022fb278e62436c1e9b1521c43cd2f0a38e47, 094bd39ab5a45fb1a728bd8e35f66b8ba74d9a1b, 1dc3cb05976fbb857938c45734768d95e1a3eeae, 098a87362b5143892d5aeb70cf01baf761cc7bf8)

[cmackenthun]

Sorry for the group ping, but am wondering if anyone has the time to review this one? Or if I'm missing a step in the contribution flow can someone let me know? Thanks!

cc @Flarna, @haddasbronfman, @hectorhdzg, @JamieDanielson, @martinkuba, @mwear, @naseemkullah, @MSNev, @pkanal, @svetlanabrennan, @trentm

[pichlermarc]

Hmm, I think I'm missing some context on the use-case of this. :thinking:

Possibly a naive theory on my end, but I'd assume that most request's bodies will contain data that's mainly important to your application logic. If that's the data that needs to be added, wouldn't it make more sense to create a span in your application and attach the attributes there?

[cmackenthun]

Thanks for taking a look @pichlermarc!

The use case for this is for when you want to add some of those attributes directly to the network span. Right now we make use of the applyCustomAttributes function to broadly add attributes to every network request that we trace in our app. Some of that includes reading the response and request body which is an async operation.

If we went the route of adding it to a span in our application then it wouldn't be directly tied to the network span that we are tracing. There's probably many ways to go about it, but to me if we're offering an interface like applyCustomAttributes that surfaces data that is received async, then just from a design perspective we should also be making the applyCustomAttributes function async to allow for that data to be used if desired

[cmackenthun]

@pichlermarc any thoughts on above? ☝️

[pichlermarc]

If we went the route of adding it to a span in our application then it wouldn't be directly tied to the network span that we are tracing.

The network span would be the parent of the application span, and they'd be part of the same trace, so the info would be visible there, right? :thinking:

There's probably many ways to go about it, but to me if we're offering an interface like applyCustomAttributes that surfaces data that is received async, then just from a design perspective we should also be making the applyCustomAttributes function async to allow for that data to be used if desired

Yes, that's understandable. :thinking: I could be completely wrong (and I understand that my opinion on it may be controversial) but I think having all these extra functions that can be passed to instrumentation that allow users to attach span attributes were somewhat of mistake. :disappointed:

From what I can tell we're going against the grain compared to what other OTel language SIGs offer in their instrumentation and it also adds adds a lot to the effort it takes to maintain such an instrumentation.

So there's a reasonably large chance that I'm just the wrong person to review this PR as I've been always failing to see the value it brings. :sweat_smile: Maybe my fellow @open-telemetry/javascript-approvers have more insights than I do. I'll also bring it up during the SIG meeting tomorrow to see if others have an idea on where this idea originally came about - I'd be happy to change my opinion in light of new info.

[pichlermarc]

So I asked around a bit what the original intent of that hook was: it's original intention was for users to be able to apply header values to their spans (which seems to be possible with the current implementation). Reading the body is generally considered to be application-specific, so we usually don't encourage doing this.

I've also reviewed other language's http-client instrumentation implementations, and in places where I found hooks to be available their stated purpose is also only to apply header values to the span.

I also read up on the semantic conventions which basically state that the span should be ended just after headers were received when reading the body of a request is optional for a library, which seems to apply to fetch.

To avoid any discrepancies, we'd therefore not await the user provided hook as it would violate the semantic conventions for HTTP client instrumentation. If data from the body is needed, we recommend creating a span in your app and attach any information there.

[trentm]

I also read up on the semantic conventions which basically state that the span should be ended just after headers were received when reading the body of a request is optional for a library, which seems to apply to fetch.

Agreed, based on this.

[pichlermarc]

Closing this PR based on the reasons mentioned above. As a workaround please use the other option (manually creating a span when handling the body and attaching the necessary data there) instead.

[cmackenthun]

@pichlermarc thanks for getting back to me! Our usage relies heavily on the custom attributes hook so I'm a little nervous to hear that it's supported use case is really only for applying headers.

I'm interested in the thinking that adding parent/child spans can solve this issue as our use case for adding a lot of attributes to spans is for when the spans get run through our span collector and we generate metrics from it. Our implementation doesn't allow for reading from parent/child spans so all the relevant data needs to be included on the spans that represent the network calls. And since the instrumentation methods are all private there's not a lot of ways outside of this hook we have to add custom attributes to the spans created by this instrumentation.

Also in terms of timing I did make changes to make sure that the timing of the spans would not be affected by waiting. And the waiting would only apply when the passed function is async so it wouldn't have any unintended side effects to users passing in custom attributes functions today? There's already a setTimeout in the method today that prevents the span from being ended right away so I can't fully understand how this would break semantic conventions more than that?