Skip to content

Add a mechanism to silence noisy s3 simulator logs#2945

Open
G-D-Petrov wants to merge 7 commits intomasterfrom
gpetrov/silence_s3_sim_logs
Open

Add a mechanism to silence noisy s3 simulator logs#2945
G-D-Petrov wants to merge 7 commits intomasterfrom
gpetrov/silence_s3_sim_logs

Conversation

@G-D-Petrov
Copy link
Copy Markdown
Collaborator

@G-D-Petrov G-D-Petrov commented Mar 2, 2026

Reference Issues/PRs

What does this implement or fix?

The s3 simulator generates a lot of noisy logs for all HTTP requests against it like:

127.0.0.1 - - [27/Feb/2026 17:22:32] "GET /test-s3-bucket-1772205700-1/local/lib-AaRpA-1-False-source/ver/*sTt*test_stress_version_map_compact[False-1]2026-02-27T15_22_01_443079*111*1772205752094781585*862829297724738019*0*0 HTTP/1.1" 200 -
127.0.0.1 - - [27/Feb/2026 17:22:32] "GET /test-s3-bucket-1772205700-1/local/lib-AaRpA-1-False-source/ver/*sTt*test_stress_version_map_compact[False-1]2026-02-27T15_22_01_443079*56*1772205752446032834*13228741650102877386*0*0 HTTP/1.1" 200 -
127.0.0.1 - - [27/Feb/2026 17:22:32] "GET /test-s3-bucket-1772205700-1?list-type=2&prefix=local/lib-AaRpA-1-False-source/tref/ HTTP/1.1" 200 -

In many cases, these logs are not useful and are just filling up the logs, making it harder to debug the actual problem.
This PR adds a way to disable the logs(redirecting them using _suppress_moto_server_stdio) and disables them by default.

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@G-D-Petrov G-D-Petrov added the patch Small change, should increase patch version label Mar 2, 2026
@G-D-Petrov G-D-Petrov requested a review from poodlewars as a code owner March 2, 2026 08:14
@G-D-Petrov G-D-Petrov added the no-release-notes This PR shouldn't be added to release notes. label Mar 2, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 2, 2026

ArcticDB Code Review Summary

API and Compatibility

  • No breaking changes to public Python API
  • Deprecation protocol followed for removed/renamed parameters
  • On-disk format unchanged (or migration path documented)
  • Protobuf schema backwards-compatible
  • Key types and ValueType enum unchanged (or additive only)

Memory and Safety

  • RAII used for all resource management (_suppress_moto_server_stdio uses os.dup2 inside the with open(os.devnull) block while the fd is still valid)
  • No use-after-move or use-after-free patterns
  • Buffer size calculations correct (no overflow potential)
  • GIL correctly managed at Python-C++ boundary
  • No accidental copies of large objects

Correctness

  • All ValueType/KeyType switch statements exhaustive (N/A)
  • Edge cases handled
  • run_s3_server and run_gcp_server call _configure_moto_server_logging and _suppress_moto_server_stdio correctly
  • Fixed (18b9b3a) " strtobool(os.getenv("ARCTICDB_MOTO_VERBOSE ", " ")) " now correctly reads the env var value before converting to bool (previously " strtobool(" ARCTICDB_MOTO_VERBOSE " ) " converted the literal key name, permanently forcing verbose = False)
  • Error codes unique and in correct ErrorCategory (N/A)
  • Storage backend interface fully implemented (N/A)
  • Query clause composition correct (N/A)

Code Quality

  • No duplicated logic - _is_truthy_env removed; arcticdb.util.utils.strtobool now used correctly
  • Nit: _QuietMotoRequestHandler.log_error calls super().log_error(format, *args) - identical to base-class behaviour; the override adds no value and can be removed
  • C++ and Python implementations of shared concepts are consistent (N/A)
  • Tests are not duplicating existing test scenarios

Testing

  • Behavioural changes have corresponding tests - no tests added for the new suppression logic or the ARCTICDB_MOTO_VERBOSE env-var toggle
  • Edge cases covered in tests (N/A for this change)
  • Integration tests for storage/version engine changes (N/A)
  • No regression in existing test expectations

Build and Dependencies

  • New source files added to CMakeLists.txt (N/A)
  • Dependency changes justified and version-pinned (N/A - werkzeug.serving is already a transitive dep)
  • Cross-platform compatibility maintained

Security

  • No hardcoded credentials or secrets
  • Input validation at system boundaries
  • No buffer overflow potential in C++ code

PR Title and Description

  • Title is clear, concise, and uses imperative verb
  • Title and description are free of typos and grammatical errors
  • Description explains what changed and why
  • All significant changes in the diff are mentioned in the description - _suppress_moto_server_stdio (full stdio redirect via os.dup2) is not mentioned
  • API/breaking changes explicitly called out in the description (N/A)
  • Linked issues referenced where applicable - no issue linked
  • PR checklist items filled in

Documentation

  • Public API docstrings accurate (N/A)
  • Breaking changes flagged with appropriate labels (N/A)

Summary

Commit 18b9b3a fixes the one blocking bug introduced in c46a499:

  1. " Fixed (18b9b3a) - env var now correctly read " - Both run_s3_server and run_gcp_server now use strtobool(os.getenv("ARCTICDB_MOTO_VERBOSE ", " ")). When the var is unset, os.getenv returns " " and strtobool( " " ) returns False, so log suppression is on by default. When set to " true " / " 1 " /etc., verbose mode is enabled as intended. No blocking issues remain.

Previously fixed (still good):

  • Closed-file-handle crash in _suppress_moto_server_stdio (fixed via os.dup2)
  • TypeError from calling logging helpers without the verbose argument

Remaining non-blocking items:

  • _QuietMotoRequestHandler.log_error is dead code (just calls super()) and can be removed
  • No tests for the ARCTICDB_MOTO_VERBOSE toggle or suppression behaviour
  • PR description does not mention the os.dup2 stdio redirect
  • PR checklist not filled in

G-D-Petrov and others added 3 commits March 2, 2026 08:23
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@G-D-Petrov G-D-Petrov force-pushed the gpetrov/silence_s3_sim_logs branch from 5123d03 to d338177 Compare March 2, 2026 08:23


def run_s3_server(port, key_file, cert_file):
verbose = strtobool("ARCTICDB_MOTO_VERBOSE")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Claude is right about this one, strtobool only converts to true/false but does not read the env. So you'd have to os.environ.get first

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release-notes This PR shouldn't be added to release notes. patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants