Consolidate logging around one process-wide logger - #154
Open
Cianidos wants to merge 14 commits into
Open
Conversation
Replace the frozen protobuf application configuration types under pkg/common/proto/stroppy with plain-Go structs and enums in pkg/config. - Define plain-Go types for run, driver, pool, logger, exporter, and isolation config, preserving the exact camelCase JSON field names and omitempty semantics. - Replace protojson and proto.Merge with strict encoding/json decoding (DisallowUnknownFields) and a deterministic per-field pool merge in internal/runner. - Generate docs/jsonschema/run.schema.json from the Go structs via a small reflection generator (internal/jsonschema-gen) with no stale proto/TS descriptions. - Migrate runner, drivers, bench APIs, workloads, and tests; keep the bench.Run/Workload/Driver surfaces stable. - Delete pkg/common/proto/stroppy, pkg/utils/protovalue, pkg/utils/protoyaml, and the unused pkg/common/logger proto helpers. - Drop the protoc-gen-validate dependency; YDB/OTLP protobuf and gRPC dependencies remain. Closes #143
Reject unknown LogLevel/LogMode values on decode (the frozen protobuf schema encoded them as strict enums), pin the remaining protojson field names and log-enum rejection in tests, derive the JSON Schema enum values from the exported constant lists, and note the global.seed quoted-number divergence in the changelog.
Exempt pkg/config camelCase json tags from tagliatelle (they are the protojson wire format), rename Go identifiers to idiomatic initialisms (SQL, URL, TLS, RunID, OtlpHTTP*) that revive var-naming flags while preserving the json tags, and fix the remaining gocritic/gosec/wsl/errcheck/ intrange/nlreturn/err113 findings in pkg/config and the jsonschema generator.
Resolve all user inputs once into typed run/workload/driver configuration and pass them through native APIs. Environment variables stay input sources but are no longer internal transport between command, bench, workload, and driver layers: - step filters (--steps/--no-steps) are explicit Run inputs, read by the bench step filter directly instead of STROPPY_STEPS/STROPPY_NO_STEPS - execute_sql SQL source is the typed sql-body/sql-file workload params (inline SQL / .sql file / sql positional bind as CLI inputs) - legacy POOL_SIZE shorthand no longer post-applies after -D extras, so explicit -D pool.maxConns / pool.minConns win deterministically - CSV workload identity comes only from the ?workload= URL option, never from STROPPY_CSV_WORKLOAD - dead STROPPY_DRIVER_N serialization and probe-env helpers removed, and pkg/bench generic environment readers (Env/EnvInt/EnvFloat) deleted - k6Args, k6Config, and driver-level defaultTxIsolation removed from the v6 config schema and rejected with migration guidance Refs #138
- add k6Args/k6Config migration guidance at config load (typed executor/vus/iterations/duration), mirroring the -D defaultTxIsolation --tx-isolation error - drop defaultTxIsolation from AGENTS.md driver-flag list - cover the defaultTxIsolation rejection with a test Refs #138
defaultInsertMethod was parsed from presets, JSON, config files, and -D but discarded by buildDriverConfig, so every workload kept its hard-coded InsertRequest.Method and invalid values passed silently. Resolve an effective insert method once per run and apply it to every load: - New run-scope typed parameter --insert-method (plus INSERT_METHOD env, -e, and config run.insertMethod) is the highest-precedence override. - The driver-level defaultInsertMethod (preset, raw JSON -d, config drivers[N], -D insertMethod/-D defaultInsertMethod) is restored through buildDriverConfig and validated with driver.ParseInsertMethod. - Unsupported-for-driver values fail before loading starts via driver.ResolveInsertMethod over the capability matrix. - The workload-authored method remains the fallback when no override is set. Precedence: typed CLI > env > config > driver default > workload default. Closes #133
A bare -d preset was leaking its defaultInsertMethod into the effective method, silently replacing the workload's hard-coded method. Presets are now driver+URL templates and carry no insert-method opinion; only --insert-method, -d/-D insertMethod/defaultInsertMethod, raw JSON, and config drivers[N] may override. Also require a resolved driver type before validating the method and capability-check the driver-set value through ResolveInsertMethod. Closes #133
Add a typed --query-timeout run parameter (QUERY_TIMEOUT env, config run.queryTimeout) that bounds each executed statement with a child context deadline; 0 (the default) disables it. Every standalone and transactional query/exec derives the deadline in the shared sqldriver path, applied per statement rather than to an entire suite. MySQL additionally binds SELECT statements server-side with the MAX_EXECUTION_TIME optimizer hint so a timed-out query keeps its pooled connection reusable; PostgreSQL relies on pgx context cancellation (CancelRequest), so no session setting can leak across pooled connections. Timeouts classify as ErrorKindTimeout, distinct from parent-run cancellation. Closes #141
- Pad the MySQL client deadline past the MAX_EXECUTION_TIME hint so the server-side 3024 fires first and keeps the pooled connection, instead of the client timer winning by ~RTT and go-sql-driver/mysql discarding it. - Bound the YDB native/COLUMNAR BulkUpsert and DescribeTable with the same per-statement deadline as the SQL arms. - Classify MySQL 3024 (ER_QUERY_TIMEOUT) as ErrorKindTimeout in tests and document the hint's SELECT-only, first-token scope. - Add a build-tag integration test blocking pg_sleep / SELECT SLEEP past the deadline and verifying timeout classification plus connection reuse.
Issue #129: consolidate logging around a single process-wide logger. The native runner started with a development/debug logger and never applied LOG_LEVEL / LOG_MODE / global.logger on the CLI path. Drivers also accepted an optional logger and otherwise built one from the environment, so every driver held its own construction path. Credential-bearing DSNs were logged raw at debug level. This change: - Replaces the atomic-swap/environment-mirror machinery in pkg/common/logger with one plain package var set once per run by logger.Init(level, mode). - Initializes that logger in the run command from --log-level/--log-mode, LOG_LEVEL/LOG_MODE, -e, and config global.logger, in that precedence, before any config/driver/workload logging. - Removes driver.Options.Logger and the per-driver NewFromEnv fallback; every driver and workload now logs through a named child of logger.Global(). - Adds logger.RedactDSN and applies it to every driver URL/DSN and config-file URL log line so credentials never print at any level. - Validates CLI/env level and mode against the pkg/config LogLevel/LogMode constants, failing fast on unknown values. Tests prove redaction, CLI>env>-e>config precedence, and that an error-level run emits no INFO/DEBUG records across the command/bench/workload/driver stack.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Cianidos
force-pushed
the
feat/issue-141-query-timeout
branch
from
August 21, 2026 23:31
9985294 to
244f55f
Compare
Contributor
Author
Retarget blocked: logger ownership and config decisions requiredThe unique #154 commit cannot be mechanically retargeted onto current A dry replay of Several functionally different choices must be made before this work can be ported:
Taking the old side would also regress current environment, step, signal, teardown, and query-timeout behavior. #152 insert-method code is unrelated and must not be reintroduced. Please choose the logger API/ownership, synchronization/reset model, exported compatibility, and protobuf default/validation behavior. Then port only process-wide logger initialization and DSN redaction onto current |
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
1 participant
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.
Implements #129.
What
One process-wide logger, configured exactly once per run before any driver or workload starts, and used (or a named child of it) everywhere.
cmd/stroppy/commands/run/run.go→initializeLogger→logger.Init(level, mode), wired after config load /-eresolution and beforebench.Run(so config-file, bench, workload, and driver logs all respect the level).--log-level/--log-mode>LOG_LEVEL/LOG_MODE>-e LOG_LEVEL/-e LOG_MODE> configglobal.logger.logLevel/logMode> defaults (info/production).atomic.Pointer/setGlobalLogger/env-mirroring machinery inpkg/common/loggerandlogger.NewFromEnv;driver.Options.Logger; the per-driver optional-logger fallback. Drivers now calllogger.Global().Named(...);bench.Runno longer takes a logger param.logger.RedactDSNmasks password/userinfo and is applied at every URL/DSN log site (postgres, mysql, picodata, ydb drivers and the config-file driver dump).Config / validation
--log-leveland--log-modeare typed flags validated against thepkg/configLogLevel/LogModeconstants (short spellingsdebug|info|warn|error|fatal,production|development), matchingLOG_LEVEL/LOG_MODE. The config file keeps the frozenLOG_LEVEL_*/LOG_MODE_*enum spellings. Invalid values fail fast.Tests
pkg/common/logger: redaction unit tests +Init/ParseModelevel/mode tests.cmd/stroppy/commands/run:resolveLoggerSettingsprecedence tests (CLI > env >-e> config > default) and an integration test that runssimpleon thenoopdriver at--log-level=errorand asserts zero INFO/DEBUG records reach the output.