feat(gax): o11y tracer helper updates

[shivanee-p]

Updates traceAttempt in TracerHelper.ts to manage span lifecycles across either Promises or streaming RPCs without closing spans prematurely.

• Add overloads of traceAttempt() function call to account for Promises or Streams returned from GAX
• Add helper functions to close spans if they are Promises or Streams
• Make sure spans only close once

Note: confirmed that the google-gax test suite passes with npm run test-application

[gemini-code-assist]

Code Review

This pull request updates constructSettings to include internalMethodName in otherArgs when telemetry is enabled, and refactors traceAttempt in TracerHelper.ts to be synchronous. The refactored traceAttempt now dynamically manages OpenTelemetry span lifecycles based on whether the traced operation returns a Promise, an EventEmitter (stream), or a synchronous value. Comprehensive unit tests have been added to verify these behaviors. There are no review comments, so I have no feedback to provide.

[shivanee-p]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors traceAttempt in TracerHelper.ts to support asynchronous span lifecycle management for both promise-based and stream-based operations, introducing helper functions handlePromise and handleStream along with comprehensive unit tests. The review feedback highlights two important improvements: first, to avoid deferring span completion to a microtask for synchronous operations, we should check if the result is a thenable and end the span synchronously if it is not; second, to prevent potential memory leaks, we should clean up the event listeners registered on the stream in handleStream once the span has ended.

[shivanee-p]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces asynchronous span lifecycle management for both Promise-based and Stream-based operations in TracerHelper.ts, along with comprehensive unit tests. However, a critical issue was identified where spans are leaked (never ended) if the traced function returns a synchronous value or an unsupported type. To resolve this, an else block should be added to immediately end the span for synchronous or unsupported return types.

[danieljbruce]

Left a couple comments. Overall, I think I need to understand the problem and architecture more broadly to understand how traceAttempt and traceHelper.ts are used.

[danieljbruce]

I don't know too much about how this code fits in with the broader architecture, but one thought I had is if we remove the listeners here which I see intends to remove the listeners applied on the stream.on('error', onError) lines of code then it is going to remove other listeners as well that we need?

I remember google-gax applies a lot of error listeners in its middleware to handle retries and other such tasks.

[shivanee-p]

I double checked, and I confirmed taht removeListener() removes only the reference for that specific callback that's used. In contrast, removeAllListeners() cleans up all of the listeners and would affect the middleware listeners.

[danieljbruce]

Could it be in scope to test retries here? I know they get complex with the streaming calls.

[shivanee-p]

Good point, thank you! I've added tests for retries in streams

[danieljbruce]

Ok. The code LGTM. I see how the traceAttempt is used as a wrapper with invokeCall. Maybe consider inlining traceAttempt into createApiCall and then refactoring pieces of it out to eliminate the need to pass a fn: () => T parameter, but that shouldn't block. Maybe just consider it as a backlog item.

The broader thing that might just be good to check is to run the google-gax tests. After the monorepo migration, I don't think they run in the CI pipeline anymore. I'll send a link on chat for how to run the gax tests. Probably would be good to run the gax tests before merging.