Emit missing ingest/merge/time, ingest/merge/cpu and ingest/persists/cpu metrics#18866
Merged
abhishekrb19 merged 6 commits intoDec 26, 2025
Merged
Conversation
- Adds the missing streaming appendator metrics for ingest/merge/time, ingest/merge/cpu and ingest/persists/cpu
Init ServiceEmitter as non-static to reuse individually for each test Add a helper in the base class for asserting segment generation metrics
abhishekrb19
changed the title
ingest/merge/time, ingest/merge/cpu and ingest/persists/cpu metrics for streaming tasksingest/merge/time, ingest/merge/cpu and ingest/persists/cpu metrics
jtuglu1
approved these changes
Dec 23, 2025
jtuglu1
left a comment
jtuglu1
left a comment
Contributor
There was a problem hiding this comment.
LGTM – left minor comments.
| @@ -748,14 +750,15 @@ public Object call() throws IOException | |||
| } | |||
| finally { | |||
| metrics.incrementNumPersists(); | |||
Contributor
There was a problem hiding this comment.
nit: can we move the increments below the time-sensitive metric emissions? Given we are reporting at nano-scale, chance of interrupt and/or delay due to atomic load+store can skew the nano count by X-XXns.
| identifier, | ||
| segment.getSize(), | ||
| indexes.size(), | ||
| (mergeFinishTime - startTime) / 1000000, |
Contributor
There was a problem hiding this comment.
👍 We should probably make sure this is not being done elsewhere. For future readers, System.nanoTime() is actually more expensive than System.getCurrentTimeMillis() so if you need milli-level precision, just use the latter from the start. Milli itself is also expensive: https://pzemtsov.github.io/2017/07/23/the-slow-currenttimemillis.html
Contributor
Author
There was a problem hiding this comment.
Yeah, there are a few other places where this nanos -> millis conversion happens, even though only millis is really required. We can clean those up together in a separate change.
airlock-confluentinc Bot
pushed a commit
to confluentinc/druid
that referenced
this pull request
Jun 7, 2026
…eb-console snapshots, stabilize BatchAppenderator timing assertion Four jobs failed on the 37.0.0-confluent PR build. All four are fork/upstream mismatches, not Confluent feature regressions: - "Other Tests" and "Druid E2E Tests" died instantly with "Could not find the selected project in the reactor". The .semaphore config was carried from druid-34 and still referenced integration-tests, integration-tests-ex/cases, integration-tests-ex/image and integration-tests-ex/tools, which upstream-37 removed (replaced by embedded-tests). Point E2E at quidem-ut,embedded-tests and drop the deleted modules from the Other Tests exclusion list. - "Web Console" failed 16 Jest snapshot tests. The squashed #378 patch carried druid-34-era web-console .snap files, overwriting druid-37's snapshots and stripping the Blueprint default props that druid-37's @blueprintjs renders (e.g. 210 lines from timezone-menu-items). Restore all affected .snap files to druid-37.0.0. console-config.js (defaultQueryContext) is left as the intentional Confluent runtime config. - "Server" failed on BatchAppenderatorTest mergeTimeMillis() > 0 (new in upstream-37 via apache#18866). On fast CI agents the sub-millisecond in-memory merge rounds to 0. Relax the wall-clock millisecond assertions to >= 0 while keeping the CPU-time (nanosecond) assertions strict, preserving the intent. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The metrics
ingest/merge/time,ingest/merge/cpuandingest/persists/cpumetrics have been documented but were previously reported as zero because they were not set in the streaming and batch appendators (it probably regressed in a refactor).These metrics are now set by the appendators and will be reported during the persist and merge phases for realtime and batch ingestion. Updated unit tests to verify that these values are emitted (non-zero values).
Also, cleaned up
KafkaIndexTaskTestandKinesisIndexTaskTestby initializing aStubServiceEmitteras a non-static member in the base classSeekableStreamIndexTaskTestBaseso it can be used by each unit test as needed.Release note
Correctly report
ingest/merge/time,ingest/merge/cpuandingest/persists/cpumetrics for streaming and batch ingestion tasks, which were previously always misreported as zero values.This PR has:
(https://github.com/apache/druid/blob/master/dev/license.md)