Skip to content

Write usage logs when diverting to spanner - #2075

Merged
n-h-diaz merged 4 commits into
datacommonsorg:masterfrom
n-h-diaz:usage-logs-dispatcher
Jul 30, 2026
Merged

Write usage logs when diverting to spanner#2075
n-h-diaz merged 4 commits into
datacommonsorg:masterfrom
n-h-diaz:usage-logs-dispatcher

Conversation

@n-h-diaz

Copy link
Copy Markdown
Contributor

Summary

Enables consistent usage logging (WriteUsageLog), x-response-id header generation, and request metadata propagation (X-Surface HTTP header) across both default and dispatcher execution paths for V2 Observation queries.

Key Changes

  • Unified Usage Logging & Header Generation (handler_v2.go):

    • Refactored V2Observation so that the diversion path assigns v2Resp and queryType rather than returning early.
    • Both default and diverted paths now share the post-execution block that generates responseId, sets x-response-id in the gRPC response headers, extracts surface and toRemote metadata from ctx and calls log.WriteUsageLog.
  • Standalone GetQueryType Helper (v2/observation/observation.go):

    • Extracted query-type classification logic into an exported helper function GetQueryType.
    • ObservationCore now computes queryType := GetQueryType(in) upfront and returns it across all branches.
    • In V2Observation, v2observation.GetQueryType(in) is called when requests are diverted.
  • Context Propagation & X-Surface Forwarding (remote/client.go & remote/datasource.go):

    • Updated all 9 RPC methods on RemoteClient to accept ctx context.Context.
    • Added a getSurface(ctx) helper to extract x-surface from the request context and forward it to util.FetchRemote(...).
    • Updated RemoteDataSource to pass ctx to all RemoteClient method invocations.
  • Test Coverage:

    • Added new unit tests in remote/client_test.go using an httptest.Server to verify X-Surface and X-Remote HTTP header forwarding through RemoteDataSource and RemoteClient.
    • Updated existing tests in handler_v2_test.go and observation_test.go to validate usage logging and GetQueryType classification.

@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 refactors the V2 observation handler to support query diversion to a dispatcher, propagates context through the remote client to forward surface metadata headers, and extracts query type determination into a reusable helper function with accompanying tests. The review feedback suggests simplifying the V2 observation handler by consolidating redundant calls to util.GetMetadata(ctx) at the beginning of the function.

Comment thread internal/server/handler_v2.go Outdated
@n-h-diaz
n-h-diaz marked this pull request as ready for review July 30, 2026 19:01
@n-h-diaz
n-h-diaz requested review from gmechali and juliawu July 30, 2026 19:01

@gmechali gmechali 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.

Thanks for fixing this!

Curious, were you abnle to run this local, or deploy this image, and confirm the logs all showed up as expevted?

@n-h-diaz

Copy link
Copy Markdown
Contributor Author

Thanks for fixing this!

Curious, were you abnle to run this local, or deploy this image, and confirm the logs all showed up as expevted?

Thanks for the review! Tested locally that responses are getting logged - there's one diff where we aren't returning place types for containedInPlace requests in the spanner response, so it's not showing up in the log entry, but this is unrelated to the logger itself (is a diff in the response), so will address as a followup

@n-h-diaz
n-h-diaz added this pull request to the merge queue Jul 30, 2026
Merged via the queue into datacommonsorg:master with commit 2fe0534 Jul 30, 2026
10 checks passed
@n-h-diaz
n-h-diaz deleted the usage-logs-dispatcher branch July 30, 2026 23:13
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