feat(observability): instrument durable checkpoint operations - #61
seonghobae wants to merge 186 commits into
Conversation
|
@coderabbitai review Review only exact current head |
|
I will not modify the branch or merge the pull request. I will verify the specified OpenTelemetry error-status contract, confidentiality, observer-failure isolation, storage-agnostic semantics, and deterministic coverage requirements. I will treat non-qualifying CI and review evidence as non-success.
|
|
@opencode-agent @cwl-noema-review Independent review-only request for exact current head |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review note for 50efd2c7e8ec2ac995925534c00c3bee24bc4b13 over stacked base ff183ecc8050e90850b55100cc12d9eb83e6c032: current OpenTelemetry Recording Errors guidance requires failed operations to set span status Error and error.type, while successful operations leave status unset. The predecessor head set bounded error.type but deliberately disabled automatic status-on-exception without replacing it with an explicit confidential Error status, so failed checkpoint spans remained Unset. A test-only RED commit 4a292443e7bcf67c50a1718106406dcec4eb16a8 captured that gap. Production now lazily resolves the host OpenTelemetry API's StatusCode.ERROR, explicitly sets it with no description only on failed operations, leaves success Unset, contains import/status observer failures, and still sanitizes span-context exit to (None, None, None). Authoritative AGENTS/CLAUDE/architecture/ADR/operator/doctoring/CHANGELOG contracts and deterministic regressions were synchronized, including OpenTelemetry semantic conventions 1.44.0 APA-7 doctoring. No branch-writing workflow or generated evidence artifact was introduced. This author review is audit evidence only and is not independent approval. Exact-head CI/Release Acceptance remain absent/queued upstream and are not counted as success; CodeRabbit exact-head full review is currently rate-limited.
|
@coderabbitai review @opencode-agent @cwl-noema-review Review-only request for exact current stacked head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_sha='f3059450f1f263ed59979ae94fb7bf5b621616be'
base_sha='90854868c7099b93087ceea07982675174112f24'
echo '=== Local commit availability ==='
git cat-file -t "$head_sha"
git cat-file -t "$base_sha"
echo '=== Commit range summary ==='
git diff --stat "$base_sha" "$head_sha"
git diff --name-status "$base_sha" "$head_sha"
echo '=== Changed Python outlines ==='
git diff --name-only "$base_sha" "$head_sha" -- '*.py' |
while IFS= read -r file; do
[ -n "$file" ] || continue
echo "--- $file"
ast-grep outline "$file" --items all --type class,function --view expanded || true
done
echo '=== OpenTelemetry and checkpoint references in changed files ==='
git diff --name-only "$base_sha" "$head_sha" |
while IFS= read -r file; do
[ -n "$file" ] || continue
rg -n -i -C 3 \
'OpenTelemetry|opentelemetry|Checkpoint|StatusCode|db\.system\.name|docker-entrypoint-initdb|semantic conventions|APA' \
"$file" || true
done
echo '=== Exact-head GitHub checks and reviews ==='
gh api "repos/ContextualWisdomLab/pg-llm-batch/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, .conclusion, .details_url] | `@tsv`' || true
gh api "repos/ContextualWisdomLab/pg-llm-batch/commits/$head_sha/status" \
--jq '.statuses[] | [.context, .state, .target_url] | `@tsv`' || true
gh pr view 61 --repo ContextualWisdomLab/pg-llm-batch \
--json headRefOid,baseRefOid,isDraft,reviews,reviewThreads,statusCheckRollup \
|| trueLength of output: 87587 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Telemetry implementation ==='
nl -ba pg_llm_batch/checkpoint_telemetry.py | sed -n '1,380p'
echo '=== Durable checkpoint-store public operation definitions ==='
ast-grep outline pg_llm_batch/checkpoint_store.py \
--items all --type class,function --match 'Checkpoint|load|save' --view expanded || true
rg -n -A35 -B8 \
'def (load|load_in_transaction|save|save_in_transaction)\s*\(' \
pg_llm_batch/checkpoint_store.py || true
echo '=== Exception-boundary and status regression tests ==='
nl -ba tests/test_checkpoint_telemetry_exception_boundary.py | sed -n '1,180p'
nl -ba tests/test_checkpoint_telemetry_span_status.py | sed -n '1,280p'
nl -ba tests/test_checkpoint_telemetry.py | sed -n '230,540p'
nl -ba tests/test_checkpoint_telemetry_storage_semantics.py | sed -n '1,160p'
echo '=== Container migration installation ==='
fd -a -i 'Dockerfile*' .
fd -a -i '*result*checkpoint*.sql' .
rg -n -C 4 'result_stream_checkpoints|docker-entrypoint-initdb\.d' . \
-g 'Dockerfile*' -g '*.sql' -g '*.yml' -g '*.yaml' || true
echo '=== PR review state and unresolved review threads ==='
gh api graphql -F owner='ContextualWisdomLab' -F name='pg-llm-batch' -F number=61 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
isDraft
headRefOid
baseRefOid
reviewDecision
reviews(first: 100) {
nodes { author { login } state commit { oid } }
}
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
comments(first: 20) { nodes { author { login } body path line } }
}
}
}
}
}' --jq '{
isDraft: .data.repository.pullRequest.isDraft,
headRefOid: .data.repository.pullRequest.headRefOid,
baseRefOid: .data.repository.pullRequest.baseRefOid,
reviewDecision: .data.repository.pullRequest.reviewDecision,
reviews: .data.repository.pullRequest.reviews.nodes,
unresolvedThreads: [
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
]
}' || trueLength of output: 2510 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Telemetry implementation ==='
cat -n pg_llm_batch/checkpoint_telemetry.py | sed -n '1,380p'
echo '=== Durable checkpoint-store public operation definitions ==='
ast-grep outline pg_llm_batch/checkpoint_store.py \
--items all --type class,function --match 'Checkpoint|load|save' --view expanded || true
rg -n -A35 -B8 \
'def (load|load_in_transaction|save|save_in_transaction)\s*\(' \
pg_llm_batch/checkpoint_store.py || true
echo '=== Exception-boundary and status regression tests ==='
cat -n tests/test_checkpoint_telemetry_exception_boundary.py | sed -n '1,180p'
cat -n tests/test_checkpoint_telemetry_span_status.py | sed -n '1,280p'
cat -n tests/test_checkpoint_telemetry.py | sed -n '230,540p'
cat -n tests/test_checkpoint_telemetry_storage_semantics.py | sed -n '1,160p'
echo '=== Container migration installation ==='
fd -a -i 'Dockerfile*' .
fd -a -i '*result*checkpoint*.sql' .
rg -n -C 4 'result_stream_checkpoints|docker-entrypoint-initdb\.d' . \
-g 'Dockerfile*' -g '*.sql' -g '*.yml' -g '*.yaml' || true
echo '=== PR review state and unresolved review threads ==='
gh api graphql -F owner='ContextualWisdomLab' -F name='pg-llm-batch' -F number=61 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
isDraft
headRefOid
baseRefOid
reviewDecision
reviews(first: 100) {
nodes { author { login } state commit { oid } }
}
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
comments(first: 20) { nodes { author { login } body path line } }
}
}
}
}
}' --jq '{
isDraft: .data.repository.pullRequest.isDraft,
headRefOid: .data.repository.pullRequest.headRefOid,
baseRefOid: .data.repository.pullRequest.baseRefOid,
reviewDecision: .data.repository.pullRequest.reviewDecision,
reviews: .data.repository.pullRequest.reviews.nodes,
unresolvedThreads: [
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
]
}' || trueLength of output: 53180 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Checkpoint migration files ==='
fd -a -i --glob '*result*checkpoint*.sql' . || true
fd -a -i --glob '*checkpoint*.sql' . || true
echo '=== Dockerfiles ==='
fd -a -i --glob 'Dockerfile*' . -x sh -c 'echo "--- $1"; cat -n "$1"' sh {} \;
echo '=== Migration installation references ==='
rg -n -C 5 'result_stream_checkpoints|docker-entrypoint-initdb\.d|checkpoint.*\.sql' . \
-g 'Dockerfile*' -g '*.sql' -g '*.yml' -g '*.yaml' || true
echo '=== Status-resolution test coverage ==='
rg -n -C 5 'StatusCode|_resolve_error_status|optional API|optional.*status|AttributeError' \
tests/test_checkpoint_telemetry*.py pg_llm_batch/checkpoint_telemetry.py
echo '=== Exact PR state and unresolved threads ==='
gh api graphql -F owner='ContextualWisdomLab' -F name='pg-llm-batch' -F number=61 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
isDraft
headRefOid
baseRefOid
reviewDecision
reviews(first: 100) {
nodes { author { login } state commit { oid } }
}
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
comments(first: 20) { nodes { author { login } body path line } }
}
}
}
}
}' --jq '{
isDraft: .data.repository.pullRequest.isDraft,
headRefOid: .data.repository.pullRequest.headRefOid,
baseRefOid: .data.repository.pullRequest.baseRefOid,
reviewDecision: .data.repository.pullRequest.reviewDecision,
reviews: .data.repository.pullRequest.reviews.nodes,
unresolvedThreads: [
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
]
}'Length of output: 38342
I found one actionable issue.
I found no other actionable defects in the requested scope. The implementation uses storage-agnostic operation spans, omits The PostgreSQL image installs the checkpoint migration at Exact-head checks succeeded for container builds, coverage/docstrings/lint/package, Python 3.10/3.12/3.14 tests, and reproducible wheel/sdist. PR
|
|
Exact-current-base ancestry audit (no branch mutation): source head The two base-only changes are already byte-identical on this source head: Do not treat the prior exact-head CI/Release Acceptance evidence as proof against the live base after any reconciliation. Reconcile the prerequisite ancestry only through a single reviewed branch-writer path, then regenerate every exact-head/exact-base gate. No temporary workflow, duplicate repair path, force update, merge, or protection bypass was created in this audit. |
|
Current head/base audit after non-force ancestry reconciliation:
The earlier |
efe2c81 to
6b2e19e
Compare
|
Superseded by #78. The direct non-force predecessor merge probe #77 was deterministically non-mergeable after #60 moved to its current linearized head. The #61 feature delta was then replayed exactly onto current #60 head |
1 participant
Product and acquisition gap
The durable result-checkpoint store provides tenant isolation, exact compare-and-swap advancement, and caller-owned transaction coupling, but operators also need storage-agnostic, low-cardinality OpenTelemetry signals that preserve confidentiality and never change checkpoint semantics.
Implemented bounded vertical slice
OpenTelemetryCheckpointStoreopt-in and compatible with package-owned or host-owned checkpoint stores;error.typevalues only;db.system.name;StatusCode.ERRORwithout a description on failed checkpoint spans when available;No branch-writing workflow, generated coverage database, build product, cache, version bump, publication, or release authority is added.
Strict red-green-refactor evidence
agent/persistent-result-checkpoint-store; current exact tip90854868c7099b93087ceea07982675174112f24(PR feat(checkpoint): persist resumable result progress #60).4a292443e7bcf67c50a1718106406dcec4eb16a8.b9f0a5a4a986d7ba678752d3bc3ecd01f081db9b.50efd2c7e8ec2ac995925534c00c3bee24bc4b13.a08f20b0e956a5bb768533888737a6150947015arequired the checkpoint migration in the fresh bundled PostgreSQL image before the Dockerfile fix.8a2deeb14c23d9db97bbad1d4c68837b6c8591ccinstalled the reviewed migration under/docker-entrypoint-initdb.d/04_result_stream_checkpoints.sql.31138736624) then exposed two valid predecessor-contract defects before feat(observability): instrument durable checkpoint operations #61 had same-head workflow evidence: the concurrency regression still expecteddb.system.name=postgresqldespite the storage-agnostic production/ADR contract, and the operator documentation missed an exact required Error-status phrase by capitalization. Those permanent tests were already RED in that integration run.b4f301e8bef7e31088e0f3eab0737ea1d685ee9ccorrected the concurrency test to enforce the intended storage-agnostic attribute set without changing production code.f3059450f1f263ed59979ae94fb7bf5b621616bealigns operator documentation with the existing explicit Error-status contract.Current exact-head staged evidence
f3059450f1f263ed59979ae94fb7bf5b621616be.90854868c7099b93087ceea07982675174112f24(agent/persistent-result-checkpoint-store). Connectorbase_shametadata may represent an older merge base and is not treated as current-base evidence.31138841164: success on the exact current head. Container builds, coverage/docstrings/lint/package, and Python 3.10/3.12/3.14 jobs all completed successfully.31138841209: success on the exact current head.COMMENTEDreview is audit history only; no qualifying independent non-author GitHubAPPROVEDreview exists.Dependency and merge boundary
Required order remains:
.github#790 -> pg-llm-batch#53 -> #55 -> #56 -> #57 -> #58 -> #59 -> #60 -> this PR.This PR remains a stacked draft. It must not be marked ready or merged until every prerequisite integrates into
main, the branch is retargeted, and fresh integrated exact-head/exact-base quality, security, dependency, packaging, provenance, supply-chain, release-acceptance, and independent-review evidence succeeds under repository protection. A qualifying independent non-author GitHubAPPROVEDreview and zero unresolved valid findings remain mandatory. No staged-base evidence is reusable as final integrated release evidence.