Skip to content

Fix REST table metric statement type labels#17757

Merged
HTHou merged 1 commit into
masterfrom
codex/fix-rest-table-metric-type
May 25, 2026
Merged

Fix REST table metric statement type labels#17757
HTHou merged 1 commit into
masterfrom
codex/fix-rest-table-metric-type

Conversation

@HTHou
Copy link
Copy Markdown
Contributor

@HTHou HTHou commented May 25, 2026

Description

This PR avoids recording full table-model AST strings in REST table v1 query metric labels.

The REST table v1 query endpoint previously passed Statement#toString() as the type label to performance_overview, which could expose long query text such as Query{queryBody=...} and produce unstable/high-cardinality labels.

The change records QUERY for the query endpoint instead. For REST table v1 non-query, this PR removes the separate EXECUTE_NON_QUERY_PLAN metric recording to keep it aligned with the RPC SQL statement path, which does not use a separate SQL non-query metric interface.

I also checked other CommonUtils.addStatementExecutionLatency call sites and did not find remaining toString() usage for metric type labels.

Verification

./mvnw -pl external-service-impl/rest -am -DskipTests -DskipITs compile

@HTHou HTHou force-pushed the codex/fix-rest-table-metric-type branch from 6d05d1f to 71ee186 Compare May 25, 2026 07:05
@HTHou HTHou force-pushed the codex/fix-rest-table-metric-type branch from 71ee186 to 50afa09 Compare May 25, 2026 07:07
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts REST table v1 statement-latency metric labeling to avoid recording full table-model AST strings (high cardinality / potentially long query text) in performance_overview tags.

Changes:

  • Record a stable metric Tag.TYPE value (QUERY) for the REST table v1 query endpoint instead of Statement#toString().
  • Remove the REST table v1 non-query EXECUTE_NON_QUERY_PLAN latency metric emission.
Comments suppressed due to low confidence (1)

external-service-impl/rest/src/main/java/org/apache/iotdb/rest/protocol/table/v1/impl/RestApiServiceImpl.java:239

  • executeNonQueryStatement no longer records performance_overview latency (the previous addStatementExecutionLatency(OperationType.EXECUTE_NON_QUERY_PLAN, …) timing block was removed). This is an observability regression: REST table v1 non-query calls will stop appearing in this metric while other REST implementations still emit EXECUTE_NON_QUERY_PLAN timers. Consider keeping the timer and switching the label to a stable, low-cardinality value (e.g., a constant like StatementType.NULL.name()/"NON_QUERY", or a bounded mapping based on statement.getClass()), instead of dropping it entirely.
    } finally {
      if (queryId != null) {
        COORDINATOR.cleanupQueryExecution(queryId);
      }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@HTHou HTHou merged commit 75d6855 into master May 25, 2026
43 of 44 checks passed
@HTHou HTHou deleted the codex/fix-rest-table-metric-type branch May 25, 2026 08:16
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.

3 participants