-
Notifications
You must be signed in to change notification settings - Fork 540
Diagnostics: Fixes GetQueryMetrics returning null inside custom RequestHandler #5879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ namespace Microsoft.Azure.Cosmos.Handlers | |||||||||||||||||||||||||||||||||||||
| using System.Linq; | ||||||||||||||||||||||||||||||||||||||
| using System.Threading; | ||||||||||||||||||||||||||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||||||||||||||||||||||||||
| using Microsoft.Azure.Cosmos.Query.Core.Metrics; | ||||||||||||||||||||||||||||||||||||||
| using Microsoft.Azure.Cosmos.Resource.CosmosExceptions; | ||||||||||||||||||||||||||||||||||||||
| using Microsoft.Azure.Cosmos.Tracing; | ||||||||||||||||||||||||||||||||||||||
| using Microsoft.Azure.Cosmos.Tracing.TraceData; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -155,6 +156,29 @@ internal async Task<ResponseMessage> ProcessMessageAsync( | |||||||||||||||||||||||||||||||||||||
| request, | ||||||||||||||||||||||||||||||||||||||
| serviceRequest.RequestContext.RequestChargeTracker); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Attach query metrics to the per-request transport trace so they are | ||||||||||||||||||||||||||||||||||||||
| // reachable from a custom RequestHandler.SendAsync as the response unwinds | ||||||||||||||||||||||||||||||||||||||
| // back through the handler pipeline (issue #5117). Before this, the datum | ||||||||||||||||||||||||||||||||||||||
| // was added in CosmosQueryClientCore.GetCosmosElementResponse, which runs | ||||||||||||||||||||||||||||||||||||||
| // after the handler pipeline has fully unwound, so handlers always saw | ||||||||||||||||||||||||||||||||||||||
| // a null GetQueryMetrics result. Guarded on operation type to avoid | ||||||||||||||||||||||||||||||||||||||
| // mis-tagging non-query operations that may ever surface a query-metrics | ||||||||||||||||||||||||||||||||||||||
| // header. The thin-client / distributed-gateway path adds the same datum | ||||||||||||||||||||||||||||||||||||||
| // from CosmosDistributedQueryClient.CreatePage (that path bypasses | ||||||||||||||||||||||||||||||||||||||
| // TransportHandler). | ||||||||||||||||||||||||||||||||||||||
| if ((request.OperationType == OperationType.Query | ||||||||||||||||||||||||||||||||||||||
| || request.OperationType == OperationType.SqlQuery) | ||||||||||||||||||||||||||||||||||||||
| && !string.IsNullOrEmpty(responseMessage?.Headers?.QueryMetricsText)) | ||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||
| string queryMetricsText = responseMessage.Headers.QueryMetricsText; | ||||||||||||||||||||||||||||||||||||||
| QueryMetricsTraceDatum queryMetricsDatum = new QueryMetricsTraceDatum( | ||||||||||||||||||||||||||||||||||||||
| new Lazy<QueryMetrics>(() => new QueryMetrics( | ||||||||||||||||||||||||||||||||||||||
| queryMetricsText, | ||||||||||||||||||||||||||||||||||||||
| IndexUtilizationInfo.Empty, | ||||||||||||||||||||||||||||||||||||||
| ClientSideMetrics.Empty))); | ||||||||||||||||||||||||||||||||||||||
| processMessageAsyncTrace.AddDatum(TraceDatumKeys.QueryMetrics, queryMetricsDatum); | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking · Correctness: Missing Guard
Add an operation-type guard:
Suggested change
Past PR #5266 (the issue that motivated centralizing the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Recommendation · Maintainability: Comment / Invariant
The thin-client / distributed-gateway path bypasses Please:
Otherwise the next person to wire a new query transport will silently double-attach and break
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Recommendation · Reliability: Trace Shape Change
The new unit tests in this PR only validate that the handler is invoked — they do not validate that aggregation still produces correct per-partition rollups. Please:
Otherwise a future trace-shape change can silently break per-partition aggregation without any signal.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Recommendation · Maintainability: Asymmetric Header Handling
The pre-PR
Both are already exposed via A small follow-up issue is fine; calling it out so it does not get forgotten.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 Suggestion · Maintainability: Design Praise (conditional) Per-partition trace as the isolation unit is elegant Once the universal-narrowing regression (sibling blocking comments) is addressed, attaching |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Enrich diagnostics context in-case of auth failures | ||||||||||||||||||||||||||||||||||||||
| if (responseMessage?.StatusCode == System.Net.HttpStatusCode.Unauthorized || responseMessage?.StatusCode == System.Net.HttpStatusCode.Forbidden) | ||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Recommendation · Maintainability: Layering
Handlersnow depends onQuery.Core.MetricsMicrosoft.Azure.Cosmos.Handlerspreviously had no dependency onMicrosoft.Azure.Cosmos.Query.Core. With thisusingthe transport handler becomes query-aware, which couples a generic transport layer to a specific feature area.Consider one of:
Query.Corenamespace (e.g.QueryMetricsTraceDatum.TryAttach(processMessageAsyncTrace, headers)) that the handler calls — preserves layering and gives you a single seam to extend forIndexUtilizationTextandQueryAdviceTextlater.IQueryMetricsExtractorvia constructor so the transport handler stays unaware of the query model.Either preserves the existing architectural boundary that has kept
Handlersthin.