Repository navigation
Add structured logs and Prometheus metrics for blobstore and packer operations - #5575
WeiQuan0605 wants to merge 3 commits into
Conversation
…lobstore and packer operations
stephanme
left a comment
stephanme
left a comment
There was a problem hiding this comment.
There are number of unrelated files touched in this PR.
…invocation Log one structured entry per storage-cli subprocess call on cc.blobstore.storage_cli_client with operation, bucket, resource_type, operation-specific fields (key/source_path/destination_key etc.), duration_seconds, exit_code, and success. Linked to vcap-request-id so all storage-cli calls for a CF API request are traceable in Kibana, comparable to CCDB statement logs. Remove unrelated delayed_jobs_log_redaction changes These changes belong to PR #5513 (query log redaction), not this observability PR.
a2c5ff2 to
7eb72ea
Compare
- Add operation and key args to storage_cli.call.finish message for quick scanning - Use labeled args (src=, dest=, key=, verb=, expires_in=) for clarity - Use File.basename to keep message concise without losing context - Log spawn errors with storage_cli.call.error including duration and error detail - Fix symbol key serialization bug in operation_log_fields (was logging :operation instead of operation) Revert user_audit_info changes unrelated to this PR
7eb72ea to
b8427c3
Compare
stephanme
left a comment
stephanme
left a comment
There was a problem hiding this comment.
I had some conversation with Claude how to centralise the metrics in run_cli.
Make run_cli the single component that guarantees "exactly one metric per invocation, on every exit path." Wrap the subprocess call in one begin/rescue with a local emitted guard. Then cp_to_blobstore keeps only the one thing that is genuinely not a CLI call: the skip.
It would make obsolete: new_cp_state, mark_run_cli_called!, mark_run_cli_succeeded!, mark_skip_metric_emitted!, the three predicate readers, perform_put_or_skip, run_put_with_fatal_rescue, handle_fatal_put_exception, emit_signal_metric_if_needed, signal_exception?, and capture_cli — roughly 70 lines of coordination logic for one net metric (the skip).
Let's discuss this offline.
|
|
||
| def capture_cli(command, args, start_time) | ||
| Open3.capture3(@cli_path, '-s', @storage_type, '-c', @config_file, *additional_flags, command, *args) | ||
| rescue StandardError => e |
There was a problem hiding this comment.
Metrics and logs are not written on Signals only on StandardError.
| fields | ||
| end | ||
|
|
||
| def capture_cli(command, args, start_time) |
There was a problem hiding this comment.
Naming: maybe better run_and_capture_cli
Might be my problem as I'm not too deep into the ruby open3 lib. But the program flow was more difficult to understand (where is storage_cli called in run_cli()?)
| return 'cp-skip' | ||
| end | ||
|
|
||
| mark_run_cli_called!(state) |
There was a problem hiding this comment.
mark_run_cli_called! is set before the CLI is actually invoked, so a pre-CLI StandardError suppresses the put failure/-3 metric.
Failure scenario: perform_put_or_skip calls mark_run_cli_called!(state) before run_put_with_fatal_rescue, i.e. before partitioned_key(destination_key) and before run_cli/Open3 ever run. If partitioned_key (or any code before Open3) raises a StandardError, run_put_with_fatal_rescue's rescue StandardError re-raises on the assumption 'run_cli already emitted a metric' (it did not), and cp_to_blobstore's rescue StandardError skips the -3 emit because run_cli_called?(state) is true. The put failure is logged as cp-error but no storage_cli metric is emitted at all — 'run_cli_called' actually means 'passed the size check', not 'CLI invoked'.
A short explanation of the proposed change:
Add structured log entries and Prometheus metrics to blobstore and packer operations.
cp_to_blobstorenow emits cp-start, cp-finish, cp-skip, and cp-error log events with destination key, duration, and blob size.cp_r_to_blobstoreemits per-phase timing logs (blobstore.cp_r.digest, .exists, .upload). Three new Prometheus metrics track storage-cli subprocess invocations:cc_storage_cli_operations_total,cc_storage_cli_duration_seconds, andcc_storage_cli_blob_size_bytes.An explanation of the use cases your change solves
Blobstore upload failures are currently silent: operators cannot tell from logs whether a droplet/package upload was attempted, skipped, or how long it took. This change makes every upload observable: structured logs allow filtering by destination key or bucket in Kibana, and Prometheus metrics enable alerting on non-zero exit codes and latency spikes across operations (put, get, exists, delete, etc.).
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests