You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "currenjin (via GitHub)" <gi...@apache.org> on 2026/04/28 02:52:04 UTC
[PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
currenjin opened a new pull request, #13850:
URL: https://github.com/apache/skywalking/pull/13850
## What
Several trace detail query DAOs accept a `@Nullable Duration` parameter but ignore it. This PR pushes the parameter down to the storage layer so that:
- **JDBC**: shard pruning via `TableHelper.getTablesForRead` (instead of scanning all TTL shards) + a `time_bucket` `WHERE` filter inside the generated SQL.
- **Elasticsearch**: a `Query.range(TIME_BUCKET)` filter on the search request.
When `duration` is `null`, both backends fall back to the prior behavior, so this is backward-compatible.
### Affected (JDBC)
- `JDBCTraceQueryDAO`: `queryByTraceId`, `queryBySegmentIdList`, `queryByTraceIdWithInstanceId`
- `JDBCSpanAttachedEventQueryDAO`: `queryZKSpanAttachedEvents`, `querySWSpanAttachedEvents`
- `JDBCZipkinQueryDAO`: `getTrace`
### Affected (Elasticsearch)
- `TraceQueryEsDAO`: `queryByTraceId`, `queryBySegmentIdList`, `queryByTraceIdWithInstanceId`
- `SpanAttachedEventEsDAO`: `querySWSpanAttachedEvents`, `queryZKSpanAttachedEvents`
## Why
`TraceQueryService.queryTrace(traceId, duration)` already passes the trace's time window to the storage DAO, but these DAOs drop it. With the default `recordDataTTL=3` and `dayStep=1`, JDBC scans 3 daily shard tables per trace detail load instead of 1; Elasticsearch sends queries that match the entire merged record index instead of the trace's time bucket range.
`BanyanDB` already handles this correctly via `getTimestampRange(duration)`, so this change brings JDBC and ES in line with BanyanDB.
The fix follows the existing pattern in the same project:
- `JDBCTraceQueryDAO.queryBasicTraces` (the conditional `getTablesForRead` / `getTablesWithinTTL` ternary + `time_bucket` `WHERE` filter)
- `JDBCBrowserLogQueryDAO.queryBrowserErrorLogs` (same pattern)
- `TraceQueryEsDAO.queryBasicTraces` (the `Query.range(TIME_BUCKET)` filter)
## Test plan
- [x] New / extended unit tests:
- `JDBCTraceQueryDAOTest`: 4 new cases covering all three methods with non-null duration + a null-duration fallback case
- `JDBCSpanAttachedEventQueryDAOTest`: 3 new cases (ZK with duration, SW with duration, ZK with null fallback)
- `JDBCZipkinQueryDAOTest`: 2 new cases (`getTrace` with duration + null fallback)
- `TraceQueryEsDAOTest` (new): 4 cases covering all three methods, with + without duration
- `SpanAttachedEventEsDAOTest` (new): 4 cases covering both methods, with + without duration
- [x] All tests in `storage-jdbc-hikaricp-plugin` and `storage-elasticsearch-plugin` pass (40+ tests)
- [x] `mvn checkstyle:check` clean for both modules
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "currenjin (via GitHub)" <gi...@apache.org>.
currenjin commented on PR #13850:
URL: https://github.com/apache/skywalking/pull/13850#issuecomment-4332245211
You're absolutely right, thanks for the careful analysis. I went back and verified, and the conclusion holds:
- `Duration.coldStage` is referenced only under `storage-banyandb-plugin/`.
- The default UI's `TraceSpans` fragment indeed sends `queryTrace(traceId)` without a `duration` variable, and the `# only for BanyanDB` comment in `trace.graphqls:200` codifies that intent.
- So on the GraphQL hot path, JDBC/ES currently take the null-fallback branch in this PR.
One concrete non-UI caller that does benefit today: `SkyWalkingTraceQLApiHandler.queryTraceImpl` (`traceql-plugin`) builds a `Duration` from the request's `start`/`end` and calls `traceQueryService.queryTrace(traceId, duration)`. So Tempo/Grafana clients hitting the SkyWalking TraceQL endpoint with JDBC/ES storage do reach the new path. (Zipkin's `getTraceById` API has no time-range argument, so it stays on the null path.)
Given that, I'd like to take **option (a)** here:
- Re-scope this PR's description to be the **storage-side plumbing fix** that brings JDBC/ES in line with BanyanDB's `getTimestampRange(duration)` and benefits non-UI duration-carrying callers (TraceQL handler, third-party GraphQL clients).
- I'll separately follow up on **option (b)** — updating the `trace.graphqls` comment and the default UI `TraceSpans` fragment so the GraphQL hot path also carries `duration` on JDBC/ES — as a follow-up that touches the `skywalking-ui` submodule and the schema, which I'd rather not bundle in this storage PR.
Updating the PR description now. Let me know if you'd prefer I close this and reopen as part of (b) instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13850:
URL: https://github.com/apache/skywalking/pull/13850#issuecomment-4332172569
The PR optimizes a path that, on JDBC/ES, the SkyWalking UI never takes — because the only thing that triggers a non-null `duration` on `queryTrace` (cold-stage mode) is a BanyanDB-exclusive feature, and BanyanDB already handled the optimization correctly via `getTimestampRange(duration)`.
Concretely:
- `Duration.coldStage` is consumed only under `storage-banyandb-plugin/`; zero references in `storage-jdbc-hikaricp-plugin/` or `storage-elasticsearch-plugin/`.
- The UI's "Cold Only" toggle (`NavBar.vue`) is the only thing that flips `coldStageMode = true`, and it's a BanyanDB-tier feature.
- With `coldStageMode = false` (the default and the only sensible state for JDBC/ES), the UI fires the `TraceSpans` fragment — `queryTrace(traceId: $traceId)` with no duration variable. So `duration` reaches the new JDBC/ES code as `null`, and the new path takes the null-fallback branch — i.e., the same `getTablesWithinTTL` scan and no `time_bucket` filter as before.
The plumbing is correct and harmless, but the headline benefit in the PR description doesn't reach the typical JDBC/ES user. Worth either (a) scoping the description to non-UI callers that do carry duration (TraceQL/Tempo handler, third-party GraphQL clients), or (b) also updating the default UI fragment + the `# only for BanyanDB` comment in `trace.graphqls` so JDBC/ES can carry duration on the hot path.
*(Analysis based on a code search by Claude Code.)*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "currenjin (via GitHub)" <gi...@apache.org>.
currenjin closed pull request #13850: Push Duration parameter down to storage layer in trace detail queries
URL: https://github.com/apache/skywalking/pull/13850
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13850:
URL: https://github.com/apache/skywalking/pull/13850#issuecomment-4332752117
From current status, I would say only attach event query with duration could be meaningful enhancement, right? But we had concerns whether the range is too strict, and attach events are rare cases, which means the query target scale is very limit, so, no condition should be fine.
That was the reason we implemented in this way.
Let me know what do you feel about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "currenjin (via GitHub)" <gi...@apache.org>.
currenjin commented on PR #13850:
URL: https://github.com/apache/skywalking/pull/13850#issuecomment-4333851999
Got it — that's the design context I was missing. Agreed on the attach event reasoning (async out-of-window arrivals + low volume = strict filter is the wrong tradeoff).
I assume the same logic applies to the trace detail queries (`queryByTraceId` etc.) — `trace_id` is selective enough that the TTL scan is acceptable. Closing this PR. Thanks for the review and the design context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Push Duration parameter down to storage layer in trace detail queries [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13850:
URL: https://github.com/apache/skywalking/pull/13850#issuecomment-4332045079
I remember the parameter of duration is actually wrong, when apply trace ID query. Have you verified how it works?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org