fix(http): negative questdb_json_queries_connections gauge #6444
fix(http): negative questdb_json_queries_connections gauge #6444bluestreak01 merged 13 commits intomasterfrom
questdb_json_queries_connections gauge #6444Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR refactors active connection tracking by introducing HTTP-specific processor identifiers (PROCESSOR_EXPORT_HTTP, PROCESSOR_ILP_HTTP), renaming tracking API methods (inc/dec/get), replacing AtomicLong with AtomicLongGaugeImpl for counters, separating metrics-facing from local gauges, and updating connection lifecycle management and processor identifiers accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nwoolmer
added
Bug
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nwoolmer
changed the title
questdb_json_queries_connections gauge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java (1)
470-479: Reset connectionCounted when you revert the increment.In the reserved-admin slot branch we roll back the earlier increment by calling
activeConnectionTracker.dec(processorName)and nulling outprocessorName, but we never clearconnectionCounted. The context therefore still believes it’s counted, so any follow-up request on the same socket will skip the!connectionCountedguard and run untracked. Please either calldecrementActiveConnectionshere or manually setconnectionCounted = falseso the internal state matches the tracker.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/io/questdb/ServerMain.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java(3 hunks)core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java(5 hunks)core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/processors/LineHttpPingProcessor.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/processors/LineHttpProcessorImpl.java(1 hunks)core/src/main/java/io/questdb/metrics/AtomicLongGauge.java(1 hunks)core/src/main/java/io/questdb/metrics/AtomicLongGaugeImpl.java(1 hunks)core/src/main/java/io/questdb/metrics/NullLongGauge.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/http/ExpParquetExportTest.java(3 hunks)core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T12:21:00.062Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 6413
File: core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java:11982-12002
Timestamp: 2025-11-19T12:21:00.062Z
Learning: QuestDB Java tests use a deterministic random seed. The test utilities (e.g., io.questdb.test.tools.TestUtils and io.questdb.std.Rnd) produce reproducible sequences, so rnd_* functions (including rnd_uuid4) yield deterministic outputs across runs. Do not flag tests in core/src/test/** that assert against values produced by rnd_* as flaky due to randomness.
Applied to files:
core/src/main/java/io/questdb/ServerMain.java
🧬 Code graph analysis (6)
core/src/main/java/io/questdb/cutlass/http/processors/LineHttpProcessorImpl.java (1)
core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (1)
ActiveConnectionTracker(34-125)
core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java (2)
core/src/main/java/io/questdb/std/Chars.java (1)
Chars(43-1646)core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (1)
ActiveConnectionTracker(34-125)
core/src/main/java/io/questdb/cutlass/http/processors/LineHttpPingProcessor.java (1)
core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (1)
ActiveConnectionTracker(34-125)
core/src/test/java/io/questdb/test/cutlass/http/ExpParquetExportTest.java (1)
core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (1)
ActiveConnectionTracker(34-125)
core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java (1)
core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (1)
ActiveConnectionTracker(34-125)
core/src/main/java/io/questdb/cutlass/http/ActiveConnectionTracker.java (2)
core/src/main/java/io/questdb/metrics/AtomicLongGaugeImpl.java (1)
AtomicLongGaugeImpl(29-71)core/src/main/java/io/questdb/metrics/NullLongGauge.java (1)
NullLongGauge(30-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (8)
core/src/main/java/io/questdb/ServerMain.java (1)
190-190: LGTM! API delegation updated correctly.The change from
getActiveConnections(processorName)tohttpServer.getActiveConnectionTracker().get(processorName)correctly aligns with the updated ActiveConnectionTracker API introduced in this PR.core/src/main/java/io/questdb/metrics/AtomicLongGauge.java (1)
28-29: LGTM! Interface extension is well-structured.The addition of
decrementAndGet()complements the existingincrementAndGet()method and provides a consistent API for atomic gauge operations. All known implementers (AtomicLongGaugeImpl and NullLongGauge) provide the required implementation.core/src/main/java/io/questdb/metrics/NullLongGauge.java (1)
44-47: LGTM! No-op implementation is consistent.The
decrementAndGet()implementation correctly returns 0, maintaining consistency with other no-op methods in NullLongGauge (e.g.,getValue(),incrementAndGet()).core/src/main/java/io/questdb/cutlass/http/processors/LineHttpPingProcessor.java (1)
46-46: LGTM! Processor identifier updated correctly.The change from
PROCESSOR_ILPtoPROCESSOR_ILP_HTTPcorrectly aligns this processor with the HTTP-specific naming scheme introduced to fix the connection gauge tracking bug. This ensures proper categorization and counting of ILP-over-HTTP connections.core/src/main/java/io/questdb/cutlass/http/processors/LineHttpProcessorImpl.java (1)
75-75: LGTM! Processor identifier updated consistently.The change to
PROCESSOR_ILP_HTTPis consistent with the HTTP-specific processor naming pattern applied across all HTTP processors in this PR.core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java (1)
242-242: LGTM! Export processor correctly categorized.The change to
PROCESSOR_EXPORT_HTTPproperly categorizes export operations with their own connection tracking, which is essential to the bug fix. Per the ActiveConnectionTracker implementation, export connections now have dedicated local gauges while using NullLongGauge for Prometheus metrics (intentional, as there's no corresponding Prometheus metric for exports).core/src/main/java/io/questdb/metrics/AtomicLongGaugeImpl.java (1)
47-50: LGTM! Implementation is correct and consistent.The
decrementAndGet()implementation correctly delegates to the underlyingAtomicLong.decrementAndGet()and mirrors the structure of the existingincrementAndGet()method.core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java (1)
142-143: LGTM! Test assertions updated correctly.All test assertions have been consistently updated to reflect the new HTTP-specific processor naming scheme:
json-http→http_json_connectionsilp-http→http_ilp_connectionsPROCESSOR_ILP→PROCESSOR_ILP_HTTPThese changes ensure tests correctly validate the updated error messages and connection tracking behavior introduced by this bug fix.
Also applies to: 247-247, 251-251, 262-262, 274-274, 279-279, 390-390, 395-395
core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java
Outdated
Show resolved
Hide resolved
[PR Coverage check]😍 pass : 34 / 38 (89.47%) file detail
|
4 participants
A bug was introduced in #6008
This bug caused the
questdb_json_queries_connectionsgauge to be decremented by sources that would not also increment the gauge. For example, 'export' and 'other' sources would also trigger a decrement.