Skip to content

feat: add configurable logging thresholds for slow db response logging #367

Open
calvinbrewer wants to merge 2 commits intomainfrom
slow-db-config
Open

feat: add configurable logging thresholds for slow db response logging #367
calvinbrewer wants to merge 2 commits intomainfrom
slow-db-config

Conversation

@calvinbrewer
Copy link
Contributor

@calvinbrewer calvinbrewer commented Feb 4, 2026

Acknowledgment

By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added configurable slow database response monitoring threshold for PostgreSQL queries. Users can now customize the duration for when slow responses are logged via environment variable, with a default of 100 milliseconds.
  • Chores

    • Version bump to 2.1.22.

Copilot AI review requested due to automatic review settings February 4, 2026 18:04
@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

A configurable threshold for slow PostgreSQL response logging is introduced. A previously hardcoded 100ms cutoff is replaced with a new configuration field CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS (defaulting to 100ms), propagated through configuration structures and exposed via accessor methods to the PostgreSQL backend for per-message logging decisions.

Changes

Cohort / File(s) Summary
Version & Metadata
.gitignore, CHANGELOG.md, Cargo.toml
Updated version to 2.1.22, documented new configurable slow DB response threshold, and adjusted git ignore entries.
Configuration Structures
packages/cipherstash-proxy/src/config/log.rs, packages/cipherstash-proxy/src/config/tandem.rs
Added new slow_db_response_min_duration_ms: u64 field with default 100ms to LogConfig, and exposed a slow_db_response_min_duration() Duration accessor on TandemConfig.
Access Layer
packages/cipherstash-proxy/src/postgresql/context/mod.rs
Added new public accessor method slow_db_response_min_duration() to forward the threshold from configuration to consumers.
Backend & Tests
packages/cipherstash-proxy/src/postgresql/backend.rs, packages/cipherstash-proxy/src/log/mod.rs
Replaced hardcoded 100ms threshold with dynamic lookup via self.context.slow_db_response_min_duration() in slow response detection; updated test setup to initialize the new field.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A magic number breaks free at last,
One hundred milliseconds bound no more,
Configuration whispers through the cast,
From config chains to logs they soar,
Now slow database reads bend to our will! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: adding configurable logging thresholds for slow database response logging, which is reflected across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch slow-db-config

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
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 adds configurable logging thresholds for slow database response logging, allowing operators to customize when warnings are logged for slow message reads from the PostgreSQL server via the CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS environment variable.

Changes:

  • Added a new configuration field slow_db_response_min_duration_ms with a default value of 100ms
  • Updated the slow database response check in the backend to use the configurable threshold instead of a hardcoded value
  • Incremented the version to 2.1.22 with corresponding CHANGELOG entry

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/cipherstash-proxy/src/config/log.rs Added new configuration field slow_db_response_min_duration_ms with default function and documentation
packages/cipherstash-proxy/src/config/tandem.rs Added accessor method to convert milliseconds to Duration
packages/cipherstash-proxy/src/postgresql/context/mod.rs Added convenience method to access the configuration from context
packages/cipherstash-proxy/src/postgresql/backend.rs Updated slow response check to use configurable threshold instead of hardcoded 100ms
packages/cipherstash-proxy/src/log/mod.rs Updated test fixture to include the new configuration field
Cargo.toml Version bump to 2.1.22
Cargo.lock Updated package versions
CHANGELOG.md Added entry for the new feature
.gitignore Added .env.proxy.dev (unrelated convenience addition)

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

Comment on lines +315 to +318
/// Returns the slow database response minimum duration as a Duration
pub fn slow_db_response_min_duration(&self) -> std::time::Duration {
std::time::Duration::from_millis(self.log.slow_db_response_min_duration_ms)
}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The new slow_db_response_min_duration method lacks test coverage. Consider adding tests similar to the existing slow_statement_accessors and slow_statements_config tests (lines 873-913) to verify that:

  1. The method correctly converts milliseconds to Duration
  2. The configuration can be loaded from environment variables (CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS)

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 7-12: Move the current "## [2.1.22] - 2026-02-05" entry into a new
top-level "[Unreleased]" section with standard Keep a Changelog subsections
(Added, Changed, Deprecated, Removed, Fixed, Security) and place the
"Configurable slow database response threshold" under Added; remove the future
release date from that heading (keep it Unreleased until 2026-02-05). Update the
comparison link references at the bottom to include the new 2.1.22/2.1.21 links
and ensure the link labels match the heading "[2.1.22]"; verify the environment
variable reference CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS remains in the Added
text but do not include a release date.
🧹 Nitpick comments (1)
packages/cipherstash-proxy/src/postgresql/backend.rs (1)

165-172: Include the configured threshold in the warning log.

This makes slow-response logs self-describing and avoids confusion when the threshold is changed from the default.

💡 Suggested tweak
-        if read_duration > self.context.slow_db_response_min_duration() {
+        let slow_threshold = self.context.slow_db_response_min_duration();
+        if read_duration > slow_threshold {
             warn!(
                 client_id = self.context.client_id,
                 msg = "Slow database response",
                 duration_ms = read_duration.as_millis(),
+                threshold_ms = slow_threshold.as_millis(),
                 message_code = ?code,
             );
         }

Comment on lines +7 to +12
## [2.1.22] - 2026-02-05

### Added

- **Configurable slow database response threshold**: The "Slow database response" log threshold is now configurable via `CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS` (default: 100ms). This controls per-message logging for individual slow reads from the PostgreSQL server.

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add an [Unreleased] section and update version links.

This changelog doesn’t include an [Unreleased] section with the required subsections, and the link references at the bottom aren’t updated for 2.1.22/2.1.21. Also, the 2.1.22 entry is dated 2026-02-05, which is in the future relative to today (2026-02-04); consider keeping this in [Unreleased] until the release date.

📌 Suggested update
@@
-## [2.1.22] - 2026-02-05
+## [Unreleased]
+
+### Added
+### Changed
+### Deprecated
+### Removed
+### Fixed
+### Security
+
+## [2.1.22] - 2026-02-05
@@
-[Unreleased]: https://github.com/cipherstash/proxy/compare/v2.1.20...HEAD
+[Unreleased]: https://github.com/cipherstash/proxy/compare/v2.1.22...HEAD
+[2.1.22]: https://github.com/cipherstash/proxy/releases/tag/v2.1.22
+[2.1.21]: https://github.com/cipherstash/proxy/releases/tag/v2.1.21

As per coding guidelines, Maintain a CHANGELOG.md file following Keep a Changelog format with [Unreleased] section and subsections for Added, Changed, Deprecated, Removed, Fixed, and Security from the user's perspective.

🧰 Tools
🪛 LanguageTool

[grammar] ~11-~11: Ensure spelling is correct
Context: ..._DB_RESPONSE_MIN_DURATION_MS` (default: 100ms). This controls per-message logging for...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 7 - 12, Move the current "## [2.1.22] -
2026-02-05" entry into a new top-level "[Unreleased]" section with standard Keep
a Changelog subsections (Added, Changed, Deprecated, Removed, Fixed, Security)
and place the "Configurable slow database response threshold" under Added;
remove the future release date from that heading (keep it Unreleased until
2026-02-05). Update the comparison link references at the bottom to include the
new 2.1.22/2.1.21 links and ensure the link labels match the heading "[2.1.22]";
verify the environment variable reference
CS_LOG__SLOW_DB_RESPONSE_MIN_DURATION_MS remains in the Added text but do not
include a release date.

Copy link
Contributor

@tobyhede tobyhede left a comment

Choose a reason for hiding this comment

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

LGTM.

Are the names clear enough?

slow_statement_min_duration_ms
slow_db_response_min_duration_ms

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.

2 participants