feat(instrumentation): allow error of safeExecuteInTheMiddleAsync to be async

[JPeer264]

👋

Which problem is this PR solving?

The safeExecuteInTheMiddleAsync is only allowing the execute function to be async. In this PR it will allow the onFinish also be async in the rare case that this needs to be done.

Currently there is no open ticket that addresses that issue, so I thought to just add the PR and see if you want to add it or not (it is a small change anyways)

FWIW: There is also a safeExecuteInTheMiddleMaybePromise in one of the contrib packages. It says it should get added here, not sure if that is something on the list (I could add it here and remove it in the contrib packages)

Short description of the changes

safeExecuteInTheMiddleAsync allows an async onFinish method that gets awaited.

Type of change

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

Not really a new feature per se, but it changes the users functionality, so I consider it as new feature.

How Has This Been Tested?

First off, I updated and fixed the tests above. setTimeout is not async and didn't do anything. Furthermore I added a done to one test to make sure the asserts have been checked, in that case the assertion could have been wrong, without a failing test.

For the new functionality I added a test that races a promise that takes 10ms. In the case the await onFinish gets changed to void onFinish, the test would fail, as the safeExecuteInTheMiddle would return the result before the second promise.

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated (I consider the TypeScript types as a documentation, since this seems to be a util function that is not documented)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JPeer264 / name: Jan Peer Stöcklmair (0299cbe, 49553bd, ed2a6ed)

[pichlermarc]

The safeExecuteInTheMiddleAsync is only allowing the execute function to be async. In this PR it will allow the onFinish also be async in the rare case that this needs to be done.

Hi. :) Implementation looks good, but I'm struggling a bit to come up with a use-case where this feature would be helpful (especially since this function does not seem to be used anywhere in our repos). Could you elaborate a bit on that?

FWIW: There is also a safeExecuteInTheMiddleMaybePromise in one of the contrib packages. It says it should get added here, not sure if that is something on the list (I could add it here and remove it in the contrib packages)

I think that's from a time where we were less strict about adding APIs that are rarely used. In the past a lot of utilities were added based on the assumption that it could become useful at some point. Nowadays that bar is a bit higher.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.20%. Comparing base (fb6476d) to head (ed2a6ed).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6032      +/-   ##
==========================================
+ Coverage   95.01%   95.20%   +0.19%     
==========================================
  Files         313      314       +1     
  Lines        8799     8865      +66     
  Branches     1888     1911      +23     
==========================================
+ Hits         8360     8440      +80     
+ Misses        439      425      -14     

see 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JPeer264]

Hi. :) Implementation looks good, but I'm struggling a bit to come up with a use-case where this feature would be helpful (especially since this function does not seem to be used anywhere in our repos). Could you elaborate a bit on that?

Sure, good point.

Overall context: I I am currently instrumenting Cloud Functions for Firebase with OpenTelemetry.

Specific use-case that brought me here: When there is a unhandled error in the Cloud Function it will kill the function immediately and it could happen that some data hasn't been yet flushed. Usually there are some hooks, such as process.on('', ...), where you can hook in and wait that everything is getting sent / shut down.

This doesn't exist on Cloud Functions for Firebase and therefore I would run a forceFlush inside this onFinish function to make sure that everything has been sent as it should. Here I actually already tried it to integrate it and it works like a charm.

[pichlermarc]

This doesn't exist on Cloud Functions for Firebase and therefore I would run a forceFlush inside this onFinish function to make sure that everything has been sent as it should. Here I actually already tried it to integrate it and it works like a charm.

Gnarly. Would've been nice to have a clean way offered by Cloud Functions for Firebase. Anyway, I think the way you did it is as close as one can get to a clean solution that situation.

LGTM then. 👍
Please also add a changelog entry in experimental/CHANGELOG.md. 🙂

[JPeer264]

Added an entry :)

[otelbot]

Thank you for your contribution @JPeer264! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.