Skip to content

feat(gax): call logNetworkAttempt and logCall in createApiCall - #9760

Open
shivanee-p wants to merge 6 commits into
shivaneep-o11y-logattemptfrom
shivaneep-o11y-add-logging-calls
Open

shivanee-p wants to merge 6 commits into
shivaneep-o11y-logattemptfrom
shivaneep-o11y-add-logging-calls

Conversation

@shivanee-p

Copy link
Copy Markdown
Contributor

This pull request integrates logNetworkAttempt and logCall into createApiCall.

Key Changes:

  • Checked checkLoggingEnabled(settings) in createApiCall and initialized staticArgs, dynamicArgs, and attemptDynamicArgs when either tracing or logging is enabled.
  • Wrapped each transport network attempt in createApiCall with logNetworkAttempt when telemetry logging is enabled, nested inside traceAttempt when tracing is also enabled so attempt logs capture the active T4 attempt span context.
  • Wrapped each logical client request in createApiCall with logCall (passing attemptDynamicArgs so rpc.method includes the RPC service and method name) when telemetry logging is enabled, nested inside traceCall when tracing is also enabled so terminal error logs capture the active T3 client request span context.

@shivanee-p
shivanee-p added this pull request to stack #9746 October 28, 2026 15:15
@shivanee-p
shivanee-p marked this pull request as ready for review October 28, 2026 15:16
@shivanee-p
shivanee-p requested a review from a team as a code owner October 28, 2026 15:16
@github-actions
github-actions Bot requested a review from danieljbruce October 28, 2026 15:16

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates telemetry logging capabilities into createApiCall by conditionally wrapping API calls and network attempts with logging functions, supported by a comprehensive suite of new unit tests. The feedback advises against weakening test coverage in loggerHelper.ts where assertions for the absence of exception fields were removed, suggesting instead to assert that these fields are undefined.

I am having trouble creating individual review comments. Click here to see my feedback.

core/packages/gax/test/unit/loggerHelper.ts (580-582)

medium

Removing these assertions weakens the test coverage for verifying that exception fields are not present in successful logs. If these assertions were removed because the keys might be present with undefined values (e.g., due to changes in the logging backend), we should instead assert that their values are undefined. This maintains robust test coverage without being fragile to the physical presence of the keys in the object.

      assert.strictEqual(otherWithoutMeta['exception.type'], undefined);
      assert.strictEqual(otherWithoutMeta['exception.message'], undefined);
      assert.strictEqual(otherWithoutMeta['exception.stacktrace'], undefined);
References
  1. Do not weaken test assertions (e.g., changing a specific minimum count assertion to a simple greater-than-zero check) solely to avoid fragility, as this reduces test coverage.

@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from 247d0f2 to c8ac3d9 Compare October 28, 2026 16:03

@quirogas quirogas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Approving to unblock—left a few minor non-blocking nits inline.

Comment thread core/packages/gax/src/createApiCall.ts Resolved
Comment thread core/packages/gax/src/createApiCall.ts Resolved
Comment on lines -580 to -582
assert.strictEqual('exception.type' in otherWithoutMeta, false);
assert.strictEqual('exception.message' in otherWithoutMeta, false);
assert.strictEqual('exception.stacktrace' in otherWithoutMeta, false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This PR deletes 3 assertions in test/unit/loggerHelper.ts (exception.type, exception.message, exception.stacktrace absence on otherWithoutMeta) that were added in #9757. Drop this unrelated hunk so #9760 stays scoped to createApiCall.ts and test/unit/apiCallable.ts.

@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from c8ac3d9 to c14d2c5 Compare October 29, 2026 16:56
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch 2 times, most recently from ef8645e to b0d358d Compare October 29, 2026 18:12
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from b0d358d to a4963ed Compare October 29, 2026 18:15
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch 2 times, most recently from d996b2e to 2860ce2 Compare October 29, 2026 18:52
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from f8f4684 to 04ef833 Compare October 29, 2026 19:46
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from 04ef833 to d557804 Compare October 29, 2026 19:58
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch 2 times, most recently from 40bbcd5 to db436d6 Compare October 29, 2026 20:35
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch 2 times, most recently from 0a572cb to c5a5f41 Compare October 30, 2026 02:12
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch 2 times, most recently from a8c285e to e1afd60 Compare October 30, 2026 04:20
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from e1afd60 to 952d3f1 Compare October 30, 2026 04:21
@danieljbruce
danieljbruce removed their request for review October 30, 2026 17:08
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from 25710ee to 94bf47b Compare October 30, 2026 20:56
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-add-logging-calls branch from 94bf47b to 9edb32a Compare October 30, 2026 23:35

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants