Add monitoring metrics for queries performance by Merteg · Pull Request #18 · VirtualInstruments/delta-sharing

Merteg · 2026-09-04T11:52:23Z

No description provided.

c93c345 shipped the metrics sources but not build.sbt, so CI failed with "object stackdriver is not a member of package io.micrometer": the code that imports it was there, the dependency that provides it was not. Adds micrometer-registry-stackdriver 1.6.5 (matching the micrometer-core Armeria 1.6.0 already puts on the classpath), with google-cloud-monitoring pinned to 3.0.4 and gax-grpc to 2.7.1 so gax stays on the 2.7.1 that google-cloud-storage requires -- the transitive default pulls gax-grpc 1.56 alongside gax 2.7.1, which do not work together. Also carries the AGENTS.md and CHANGELOG-VIRTANA.md entries omitted from the same commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… cdf query class's unattributed residual

Copilot

🟡 Changes recommended

Authentication coverage, classification, signed-file counts, rollout configuration, and documentation contain unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds query-performance telemetry through Micrometer and Google Cloud Monitoring, including request-level and query-stage metrics.

Changes:

Adds query classification, request decorators, metrics registries, and configuration.
Instruments CDF response construction and query completion.
Adds tests, deployment configuration, dependencies, and documentation.

File summaries

File	Description
`server/src/test/scala/io/delta/sharing/server/telemetry/RequestMetricsServiceSuite.scala`	Tests status, outcome, and timeout helpers.
`server/src/test/scala/io/delta/sharing/server/telemetry/RequestMetricsIntegrationSuite.scala`	Tests the Armeria metrics decorator.
`server/src/test/scala/io/delta/sharing/server/telemetry/QueryMetricsSuite.scala`	Tests emitted query and request metrics.
`server/src/test/scala/io/delta/sharing/server/telemetry/QueryClassSuite.scala`	Tests query classification.
`server/src/test/scala/io/delta/sharing/server/telemetry/MetricsRegistriesSuite.scala`	Tests registry configuration and labels.
`server/src/test/scala/io/delta/sharing/server/config/MetricsConfigSuite.scala`	Tests metrics configuration validation.
`server/src/main/scala/io/delta/standalone/internal/DeltaSharedTable.scala`	Measures CDF response construction.
`server/src/main/scala/io/delta/sharing/server/telemetry/RequestMetrics.scala`	Adds request context and decorator metrics.
`server/src/main/scala/io/delta/sharing/server/telemetry/QueryMetrics.scala`	Implements Micrometer metrics recording.
`server/src/main/scala/io/delta/sharing/server/telemetry/QueryClass.scala`	Defines the query taxonomy.
`server/src/main/scala/io/delta/sharing/server/telemetry/MetricsRegistries.scala`	Configures Cloud Monitoring export.
`server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala`	Integrates metrics into server requests.
`server/src/main/scala/io/delta/sharing/server/DeltaSharedTableProtocol.scala`	Extends CDF timing data.
`server/src/main/scala/io/delta/sharing/server/config/ServerConfig.scala`	Adds `MetricsConfig`.
`memory-bank/README.md`	Links the metrics design.
`memory-bank/08-query-performance-metrics.md`	Documents design and rollout.
`memory-bank/01-overview.md`	Documents configuration.
`manifests/zing-preview/configmap.yaml`	Configures preview metrics.
`manifests/zcloud-prod3/configmap.yaml`	Configures prod3 metrics.
`manifests/zcloud-prod2/configmap.yaml`	Configures prod2 metrics.
`manifests/zcloud-prod/configmap.yaml`	Configures production metrics.
`manifests/zcloud-emea/configmap.yaml`	Configures EMEA metrics.
`manifests/base/deployment.yaml`	Exposes the pod name as task ID.
`manifests/base/configmap.yaml`	Adds base metrics configuration.
`CHANGELOG-VIRTANA.md`	Records the Virtana divergence.
`build.sbt`	Adds Micrometer and monitoring dependencies.
`AGENTS.md`	Adds metrics development guidance.

Review details

Suppressed comments (1)

server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala:422

This count includes endStreamAction whenever paging, refresh tokens, or the end-stream capability is used. A one-file response with protocol, metadata, file, and end action is therefore reported as two signed URLs; Kernel deletion-vector URLs can also make action count differ from URL count. Pass the actual signing count from the query implementation.

      signedUrls = numSigned,

Files reviewed: 26/27 changed files
Comments generated: 15
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Decorator ordering (group 2). The metrics decorator was registered before the authorization decorator, and Armeria runs the most recently registered decorator outermost -- so auth rejections never reached it and every 401 was silently absent from the metrics while the error rate looked perfect. Verified both ways before and after the fix; RequestMetricsIntegrationSuite now pins it down. Metric accuracy and contracts (group 3): - files_signed no longer derives the count from actions.length - 2, which also counts protocol, metadata (historical metadata included) and the end-stream action. QueryResult.signedFiles carries the real count out of the standalone and CDF paths; the Kernel path reports None and the metric is skipped rather than guessed. - Query classification now runs before the body validation instead of after it, so a rejected request is no longer attributed to the route-derived class and counted against snapshot's 4xx breakdown. - jsonPredicateHints-only queries classify as snapshot_filtered. They are filtered queries even though the Kernel engine serves them, so this class now legitimately appears with engine=kernel. - Async-query acknowledgements classify as Other, keeping their trivial latency out of the real query distributions, as QueryClass.Other already documented. - The version and metadata handlers set the tenant attribute, so share-scoped requests are no longer emitted as tenant="unknown" when the label is enabled. - MicrometerQueryMetrics enforces the no-throw contract the trait documents: every public recorder call is guarded, logging once. These calls sit in the request path, so a telemetry failure must not fail a customer query. Documentation (group 4). Records the response_build stage added in a98b351 in the changelog, the design doc and the stage lists, and rewords the AGENTS.md instrumentation rule: a real stage boundary belongs in the timing case classes, what to avoid is an ad-hoc nanoTime feeding only a log line. The rollout state is now a per-environment table -- zing-dev, zing-preview and zcloud-prod enabled, prod2/prod3/emea not -- instead of the stale "disabled everywhere". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

CDF validation loses tenant attribution, and the async query-status route is incorrectly classified as catalog traffic.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 26/27 changed files
Comments generated: 2
Review effort level: Balanced

The CDF handler set its query class and tenant after the maxFiles check, so a rejected CDF request was recorded as tenant="unknown". This is the same defect fixed in listFiles in 2aecb04 -- the second handler was missed. Both now label before any validation can throw, as does getQueryStatus. QueryClass.forRoute sent the async query-status route (/tables/{table}/queries/{queryId}) to the catalog bucket via the broad "/shares" fallback. That route reads table state, while catalog is otherwise sub-second config lookups, so mixing them made the catalog distribution unreadable. It now classifies as Other, matching the async acknowledgement path, and the handler sets the class explicitly as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bharatZen

Re. the tenant label approach:
Does this have to be all or nothing? We need the consolidated metrics across all tenants to set standard ops alerts on etc. However, we may want to turn on tracking for specific tenants that may be temporarily encountering issues etc. Ii would be useful to see if this could be done in addition to capturing the standard consolidated form (since ops alerts etc. are based on that), so that perf for these tenants can be tracked for better diagnosis etc. without impacting the normal ops. This can be considered in a separate Jira, the current implementation is good for this one.

Merteg · 2026-09-09T07:18:17Z

Re. the tenant label approach: Does this have to be all or nothing? We need the consolidated metrics across all tenants to set standard ops alerts on etc. However, we may want to turn on tracking for specific tenants that may be temporarily encountering issues etc. Ii would be useful to see if this could be done in addition to capturing the standard consolidated form (since ops alerts etc. are based on that), so that perf for these tenants can be tracked for better diagnosis etc. without impacting the normal ops. This can be considered in a separate Jira, the current implementation is good for this one.

https://virtualinstruments.atlassian.net/browse/ZING-45509

Merteg and others added 3 commits September 2, 2026 17:27

Add monitoring metrics for queries performance

c93c345

New stage: response_build — closes one of the untimed gaps behind the…

a98b351

… cdf query class's unattributed residual

Copilot AI balanced review requested due to automatic review settings September 4, 2026 11:52

Copilot started reviewing on behalf of Merteg September 4, 2026 11:52 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Merteg requested a balanced review from Copilot September 4, 2026 14:28

Copilot started reviewing on behalf of Merteg September 4, 2026 14:28 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala Outdated

Comment thread server/src/main/scala/io/delta/sharing/server/telemetry/QueryClass.scala

bharatZen approved these changes Sep 8, 2026

View reviewed changes

Merteg merged commit 3a0cf07 into master Sep 9, 2026
10 checks passed

Merteg deleted the ZING-45330 branch September 9, 2026 07:19

Navigation Menu

Uh oh!

Add monitoring metrics for queries performance - #18

Add monitoring metrics for queries performance#18
Merteg merged 5 commits into
masterfrom
ZING-45330

Merteg commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

bharatZen left a comment •

edited

Loading

Uh oh!

Merteg commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Merteg commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

bharatZen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Merteg commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bharatZen left a comment •

edited

Loading