feat: add performance instrumentation for IPFS/IPNS and TEE operations#281
Merged
Conversation
Entire-Checkpoint: be66316e94c4
Entire-Checkpoint: c149362e8de0
Entire-Checkpoint: a92afb4c5dce
Entire-Checkpoint: 60111eaf9d00
…istograms - Tests for cipherbox_ipfs_ipns_duration_seconds histogram registration and labels - Tests for cipherbox_republish_batch_duration_seconds histogram registration and labels - Tests for startTimer pattern on both new histograms - Regression test for existing httpRequestDuration histogram Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 01a44c88faed
… MetricsService - Register cipherbox_ipfs_ipns_duration_seconds with operation/result/source labels - Register cipherbox_republish_batch_duration_seconds with tee_provider/result labels - IPFS/IPNS buckets: 1ms-30s exponential spacing for mixed fast/slow operations - Republish buckets: 1s-120s extended tier for batch processing - All 9 unit tests pass including regression for existing httpRequestDuration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e79cab4fba27
…stograms - IpnsService: timing wrappers on resolveRecord (operation=resolve, source=db/network) and publishRecord (operation=publish) - IpfsController: timing wrappers on pinFile (operation=pin) and getFile (operation=cat) - RepublishProcessor: timing wrapper on processRepublishBatch (tee_provider=mock) - All result labels track success/error for each operation - 12 new timing-specific tests across 3 test files - All 689 API tests pass with zero regressions - Regenerated API client (openapi.json formatting update) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2444127b8d8f
- Created 18-01-SUMMARY.md with execution results - Updated STATE.md with plan progress and key decisions - Updated ROADMAP.md with phase 18 plan progress (1/2) - Marked PERF-01, PERF-02, PERF-04 requirements complete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 562a4e8e3785
…panels - Add prometheus.scrape "kubo" block targeting ipfs:5001/debug/metrics/prometheus - Add IPFS Pin/Cat Duration and Error Rate panels in File Operations section - Add IPNS Resolve/Publish Duration and Source breakdown panels in IPNS section - Add TEE Republish Batch Duration panel in TEE section - Add Kubo Health row with Peer Connections, Bandwidth, and Datastore panels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 84821478c0d0
…template - Create scripts/baseline-benchmark.sh for reproducible staging benchmarks - Script exercises IPNS resolve, IPFS upload, and IPFS download with 20 iterations - Computes and prints p50/p95/p99 percentiles in markdown table format - Create .planning/baselines/18-performance-baselines.md template with TBD values - Baselines document covers client-side, server-side, HTTP API, and Kubo health metrics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 666825258a50
- Add 18-02-SUMMARY.md with execution results - Update STATE.md: Phase 18 complete (2 of 2 plans), 157 total plans - Update ROADMAP.md: Phase 18 progress to Complete - Mark PERF-03 requirement complete in REQUIREMENTS.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b27d9ffa94db
- Mark Task 3 checkpoint as approved (3/3 tasks complete) - Update ROADMAP.md progress for Phase 18 (Complete) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 366859335d38
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughPhase 18 completed: added Prometheus histograms and timing wrappers for IPFS/IPNS/republish, instrumented services and tests, enabled Kubo Prometheus scraping and expanded Grafana staging dashboard, added a baseline benchmark script and baselines doc, and updated planning/status files. Changes
Sequence DiagramsequenceDiagram
actor Client
participant IpfsCtrl as IPFS Controller
participant IpnsService as IPNS Service
participant Metrics as Metrics Service
participant Prom as Prometheus
participant DB as DB
rect rgba(100,200,150,0.5)
Client->>IpfsCtrl: POST /pin (file)
IpfsCtrl->>Metrics: startTimer({operation:'pin', source:''})
IpfsCtrl->>IpfsCtrl: pinFile()
IpfsCtrl->>Metrics: endTimer({result:'success'})
Metrics->>Prom: emit histogram observation
IpfsCtrl-->>Client: 200 + CID
end
rect rgba(100,150,200,0.5)
Client->>IpnsService: resolveRecord()
IpnsService->>Metrics: startTimer({operation:'resolve'})
IpnsService->>IpnsService: delegatedRouting.resolve() (network)
alt network returns newer
IpnsService->>Metrics: endTimer({result:'success', source:'network'})
else network fails or older
IpnsService->>DB: lookup cache
IpnsService->>Metrics: endTimer({result:'success', source:'db'})
end
Metrics->>Prom: emit histogram observation
IpnsService-->>Client: IPNS record
end
rect rgba(200,150,100,0.5)
Client->>IpnsService: publishRecord() (batch)
IpnsService->>DB: upsert record
IpnsService->>Metrics: startTimer({tee_provider:'mock'})
IpnsService->>IpnsService: delegated publish (best-effort)
IpnsService->>Metrics: endTimer({result:'success'})
Metrics->>Prom: emit batch histogram observation
IpnsService-->>Client: publish response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
==========================================
+ Coverage 47.96% 48.13% +0.16%
==========================================
Files 109 109
Lines 8358 8387 +29
Branches 652 652
==========================================
+ Hits 4009 4037 +28
Misses 4177 4177
- Partials 172 173 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end performance instrumentation for IPFS/IPNS operations and TEE republishing, plus staging observability assets (Alloy scrape + Grafana dashboard + baseline script) to establish Phase 18 baselines.
Changes:
- Added Prometheus histograms for IPFS/IPNS durations and TEE republish batch durations, and instrumented API code paths with
startTimer(). - Extended staging monitoring: Alloy scrapes Kubo metrics; Grafana dashboard includes new latency/error panels and a Kubo health row.
- Added a synthetic baseline benchmark script and Phase 18 planning/baseline documentation updates.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/metrics/metrics.service.ts | Registers new duration histograms in the Prometheus registry. |
| apps/api/src/metrics/metrics.service.spec.ts | Verifies histogram registration/labels and startTimer observability. |
| apps/api/src/ipns/ipns.service.ts | Instruments IPNS publish/resolve timing with ipfsIpnsDuration labels. |
| apps/api/src/ipns/ipns.service.spec.ts | Adds timing-specific tests for resolve/publish instrumentation. |
| apps/api/src/ipfs/ipfs.controller.ts | Instruments IPFS pin (upload) and cat (download) timing. |
| apps/api/src/ipfs/ipfs.controller.spec.ts | Adds timing-specific tests for pin/cat instrumentation. |
| apps/api/src/republish/republish.processor.ts | Instruments republish batch processing duration with republishBatchDuration. |
| apps/api/src/republish/republish.processor.spec.ts | Adds timing-specific tests for republish batch instrumentation. |
| docker/alloy-config.river | Adds Kubo /debug/metrics/prometheus scrape target. |
| docker/grafana/dashboards/cipherbox-staging.json | Adds new IPFS/IPNS/TEE duration panels + error rate + Kubo health row. |
| scripts/baseline-benchmark.sh | Adds reproducible client-side timing benchmark for staging baselines. |
| .planning/baselines/18-performance-baselines.md | Adds template for recording Phase 18 baseline measurements. |
| .planning/phases/18-performance-instrumentation/18-CONTEXT.md | Phase context/decisions for perf instrumentation scope. |
| .planning/phases/18-performance-instrumentation/18-RESEARCH.md | Research notes and implementation rationale for Phase 18. |
| .planning/phases/18-performance-instrumentation/18-VALIDATION.md | Validation strategy for Phase 18 tasks. |
| .planning/phases/18-performance-instrumentation/18-01-PLAN.md | Plan for implementing API-side histograms + instrumentation. |
| .planning/phases/18-performance-instrumentation/18-01-SUMMARY.md | Summary of Plan 01 execution and decisions. |
| .planning/phases/18-performance-instrumentation/18-02-PLAN.md | Plan for dashboards, Kubo scraping, and baseline capture artifacts. |
| .planning/phases/18-performance-instrumentation/18-02-SUMMARY.md | Summary of Plan 02 execution and decisions. |
| .planning/config.json | Planning config tweak (adds _auto_chain_active). |
| .planning/STATE.md | Updates milestone state to reflect Phase 18 completion. |
| .planning/ROADMAP.md | Marks Phase 18 complete and updates progress tracking. |
| .planning/REQUIREMENTS.md | Marks PERF-01..04 complete. |
You can also share your feedback on Copilot code review. Take the survey.
- Set timerResult='error' for non-BAD_GATEWAY exceptions in resolveRecord - Use try/catch/finally for republish batch timer to ensure observation - Filter dashboard PromQL to result="success" for latency percentiles - Validate numeric time_total in benchmark script before recording - Remove unused PUBLISH_ITERATIONS variable - Redact vault response from benchmark script error output - Fix ROADMAP progress table column alignment and plan checkboxes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4ce82610e9e5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docker/grafana/dashboards/cipherbox-staging.json (1)
1963-1974: Aggregate the raw Kubo libp2p series before plotting node-level totals.These expressions currently chart the raw exported series behind fixed legends. If Kubo exposes per-transport or per-direction labels here, Grafana will render duplicate/partial lines instead of a single node-level value. Wrapping them in
sum(...)/sum(rate(...))makes the panels match their titles and is safer across Kubo metric variants. Please verify the staged Kubo scrape before shipping the dashboard.📈 Suggested PromQL shape
- "expr": "libp2p_swarm_connections_opened_total - libp2p_swarm_connections_closed_total", + "expr": "sum(libp2p_swarm_connections_opened_total{job=\"kubo\"}) - sum(libp2p_swarm_connections_closed_total{job=\"kubo\"})", - "expr": "rate(libp2p_network_out_bytes_total[$__rate_interval])", + "expr": "sum(rate(libp2p_network_out_bytes_total{job=\"kubo\"}[$__rate_interval]))", - "expr": "rate(libp2p_network_in_bytes_total[$__rate_interval])", + "expr": "sum(rate(libp2p_network_in_bytes_total{job=\"kubo\"}[$__rate_interval]))",Also applies to: 2047-2058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/grafana/dashboards/cipherbox-staging.json` around lines 1963 - 1974, The panel is plotting raw series and can show duplicate/partial lines when Kubo exposes per-transport/direction labels; change the expressions to aggregate node-level totals by wrapping the metrics with sum(...) (e.g., replace the current libp2p_swarm_connections_opened_total and libp2p_swarm_connections_closed_total usage with a summed expression such as sum(libp2p_swarm_connections_opened_total) - sum(libp2p_swarm_connections_closed_total) and replace go_goroutines{job="kubo"} with sum(go_goroutines{job="kubo"})), and apply the same aggregation to the other affected panel expressions around the go_goroutines / libp2p usages noted in the diff (the metrics libp2p_swarm_connections_opened_total, libp2p_swarm_connections_closed_total, and go_goroutines{job="kubo"}); after updating, verify against the staged Kubo scrape to ensure labels are aggregated as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 42-89: The publishRecord path is instrumented with
metricsService.ipfsIpnsDuration.startTimer but publishBatch duplicates the
upsert + delegatedRouting.publish logic and thus never records per-record
publish timings; extract the per-record publish flow into a shared helper (e.g.,
a new private method like doPublishRecord or reuse publishRecord logic) that
performs: base64 decode, upsertFolderIpns call, delegatedRouting.publish
(best-effort) and starts/ends the metrics timer
(metricsService.ipfsIpnsDuration.startTimer with operation='publish') around the
whole per-record work so both publishRecord and publishBatch call that helper
for each item, ensuring endTimer is invoked with the correct result for every
record.
- Around line 312-379: The histogram currently defaults timerResult to 'success'
and never flips it for exceptions thrown outside the BAD_GATEWAY path, so
failures are underreported; wrap the main resolve logic (the existing try block
that calls delegatedRouting.resolve and folderIpnsRepository.findOne) with a
catch that sets timerResult = 'error' and rethrows the error before the existing
finally, i.e. change the current "try { ... } finally { endTimer(...)}" to "try
{ ... } catch (err) { timerResult = 'error'; throw err; } finally { endTimer({
result: timerResult, source: timerSource }); }" so any exception from
delegatedRouting.resolve, folderIpnsRepository.findOne, the sequence comparison,
or parseIpnsRecordBytes is recorded as an error.
In `@scripts/baseline-benchmark.sh`:
- Around line 81-88: Update timed_curl to capture both %{http_code} and
%{time_total} (e.g. curl -s -o /dev/null -w "%{http_code} %{time_total}") and
split the output into http_code and time_total; only append time_total to the
output_file when http_code is a 2xx (starts with "2"); if not 2xx, do not write
the timing and return a non-zero status. Apply the same change to the upload
path that writes to "$UPLOAD_FILE" so it also checks %{http_code} before
appending and excludes failed iterations.
- Around line 15-25: The script baseline-benchmark.sh currently accepts the JWT
as a positional argument and prints the full /vault response; change it to read
the token from a secure environment variable (e.g. CIPHERBOX_JWT) or prompt
interactively with a silent read (-s) rather than using a CLI arg, update the
usage text accordingly, and remove any direct echo of the token or embedding it
in process-visible args; for error handling around the /vault curl call (the
code that POSTs/GETs vault discovery), parse the JSON response and redact
sensitive fields (e.g. encryptedRootIpnsPrivateKey, encryptedRootFolderKey)
before logging — only log minimal context such as HTTP status, a
truncated/hashed identifier, and a short error message instead of the full
payload.
- Around line 95-96: The script reads the wrong JSON key from VAULT_RESPONSE:
change the extraction that sets IPNS_NAME to use the 'rootIpnsName' field
instead of 'ipnsName' (update the python3 JSON get call that sets IPNS_NAME to
.get('rootIpnsName','')), so IPNS_NAME is populated for initialized vaults and
the IPNS resolve baseline runs; keep the same error-suppression/|| true behavior
and stdout handling as in the existing VAULT_RESPONSE and IPNS_NAME assignments.
---
Nitpick comments:
In `@docker/grafana/dashboards/cipherbox-staging.json`:
- Around line 1963-1974: The panel is plotting raw series and can show
duplicate/partial lines when Kubo exposes per-transport/direction labels; change
the expressions to aggregate node-level totals by wrapping the metrics with
sum(...) (e.g., replace the current libp2p_swarm_connections_opened_total and
libp2p_swarm_connections_closed_total usage with a summed expression such as
sum(libp2p_swarm_connections_opened_total) -
sum(libp2p_swarm_connections_closed_total) and replace go_goroutines{job="kubo"}
with sum(go_goroutines{job="kubo"})), and apply the same aggregation to the
other affected panel expressions around the go_goroutines / libp2p usages noted
in the diff (the metrics libp2p_swarm_connections_opened_total,
libp2p_swarm_connections_closed_total, and go_goroutines{job="kubo"}); after
updating, verify against the staged Kubo scrape to ensure labels are aggregated
as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac524b1f-8931-4390-999f-fdf923f74787
📒 Files selected for processing (23)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/baselines/18-performance-baselines.md.planning/config.json.planning/phases/18-performance-instrumentation/18-01-PLAN.md.planning/phases/18-performance-instrumentation/18-01-SUMMARY.md.planning/phases/18-performance-instrumentation/18-02-PLAN.md.planning/phases/18-performance-instrumentation/18-02-SUMMARY.md.planning/phases/18-performance-instrumentation/18-CONTEXT.md.planning/phases/18-performance-instrumentation/18-RESEARCH.md.planning/phases/18-performance-instrumentation/18-VALIDATION.mdapps/api/src/ipfs/ipfs.controller.spec.tsapps/api/src/ipfs/ipfs.controller.tsapps/api/src/ipns/ipns.service.spec.tsapps/api/src/ipns/ipns.service.tsapps/api/src/metrics/metrics.service.spec.tsapps/api/src/metrics/metrics.service.tsapps/api/src/republish/republish.processor.spec.tsapps/api/src/republish/republish.processor.tsdocker/alloy-config.riverdocker/grafana/dashboards/cipherbox-staging.jsonscripts/baseline-benchmark.sh
…ield - Check HTTP status code in timed_curl — skip non-2xx responses to avoid corrupting baseline percentiles with error timings - Fix vault field name: rootIpnsName (not ipnsName) per VaultResponseDto Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f2605cf0c2f8
…tation publishBatch() had an inline copy of the publish flow (base64 decode, upsertFolderIpns, delegatedRouting.publish). Refactored to delegate to publishRecord() per entry, which eliminates the duplicated code path and ensures batch publishes emit per-record duration histogram observations via the existing instrumentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 84ccf95d0a09
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
…ilter - Upload timing loop now checks HTTP status (2xx) before recording timings and extracting CIDs, matching the timed_curl pattern - Republish batch duration percentile queries now filter to result="success" to avoid skewing p50/p95/p99 with error durations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 71992db82ebb
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/republish/republish.processor.ts (1)
25-49:⚠️ Potential issue | 🟠 MajorMark fully failed republish runs as
error.
processRepublishBatch()can returnprocessed === failedwhen the TEE is unreachable without throwing, sobatchResultstays"success"today. That means outage batches are included in the new success-onlycipherbox_republish_batch_duration_secondsquantiles, which makes the panel look healthier than the underlying run actually was.Suggested fix
try { const result = await this.republishService.processRepublishBatch(); this.metricsService.republishEntriesProcessed.inc({ result: 'succeeded' }, result.succeeded); this.metricsService.republishEntriesProcessed.inc({ result: 'failed' }, result.failed); this.logger.log( `Republish job complete: processed=${result.processed}, succeeded=${result.succeeded}, failed=${result.failed}` ); // If all entries failed and none succeeded, TEE might be down. // When TEE recovers, reactivate stale entries. if (result.processed > 0 && result.succeeded === 0 && result.failed === result.processed) { + batchResult = 'error'; this.logger.warn('All republish entries failed. TEE worker may be unreachable.'); } } catch (error) { batchResult = 'error';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/republish/republish.processor.ts` around lines 25 - 49, processRepublishBatch can return processed===failed without throwing, so batchResult remains "success" today; update the try block after receiving result (the same place where the code currently logs the "All republish entries failed" warning) to set batchResult = 'error' when result.processed > 0 && result.succeeded === 0 && result.failed === result.processed so endBatchTimer({ result: batchResult }) reports "error"; keep existing metricsService.republishEntriesProcessed increments and logger.warn in place but ensure batchResult is flipped before the finally block calls endBatchTimer.
♻️ Duplicate comments (1)
apps/api/src/ipns/ipns.service.ts (1)
289-350:⚠️ Potential issue | 🟠 MajorDB/cache failures can still be recorded as
success.The inner catch only covers delegated-routing work. If
folderIpnsRepository.findOne()or eitherBigInt()conversion throws,finallystill emitsresult="success".🩹 Minimal fix
- return null; - } finally { + return null; + } catch (error) { + timerResult = 'error'; + throw error; + } finally { endTimer({ result: timerResult, source: timerSource }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/ipns/ipns.service.ts` around lines 289 - 350, The finally block calls endTimer using timerResult which can remain "success" if an exception occurs after the delegated-routing catch (e.g., folderIpnsRepository.findOne(), BigInt conversions, or parseIpnsRecordBytes); expand the error handling so any exception in the whole resolve flow sets timerResult='error' before rethrowing. Wrap the DB/cache lookup and sequence-number comparisons (calls to folderIpnsRepository.findOne and the BigInt(...) conversions used to compare sequence numbers) in a try/catch (or move the delegated-routing try to cover the whole block) that sets timerResult='error' and then throws the error, ensuring endTimer always records error on failures; keep references to endTimer, timerResult, timerSource, folderIpnsRepository.findOne, BigInt, and delegatedRouting.resolve to locate the changes.
🧹 Nitpick comments (1)
apps/api/src/ipns/ipns.service.ts (1)
55-61: Avoidatob()for binary decode.
atob()materializes the record as a string first. Decode directly to bytes instead so this path stays inUint8Arrayform end-to-end.As per coding guidelines "Use
Uint8Arrayfor all binary data instead of strings to ensure proper binary handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/ipns/ipns.service.ts` around lines 55 - 61, The current code uses atob() to decode dto.record into a string then to bytes (recordBytes), which breaks the end-to-end Uint8Array requirement; replace the atob-based path with a direct base64-to-bytes conversion (e.g., use Buffer.from(dto.record, 'base64') or other direct base64 decoder) and assign its Uint8Array result to recordBytes, keeping the existing try/catch and BadRequestException on failure; update references to atob(), recordBytes, and dto.record in the method in ipns.service.ts accordingly so binary data remains a Uint8Array throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 75-82: The publish flow currently treats DB-save success as an
overall "success" metric even when delegatedRouting.publish(dto.ipnsName,
recordBytes) throws; update the catch blocks around delegatedRouting.publish to
record a distinct metric label (e.g. result="delegated_error" or
result="partial_success") instead of emitting result="success" — change the
metric increment/observe call inside the catch for delegatedRouting.publish to
use the delegated-failure label, and apply the same change to the second catch
(lines 89-93) so delegated-routing outages are visible while still preserving
DB-backed behavior; locate the delegatedRouting.publish call and its surrounding
metric emission in the publish handler to make this change.
In `@docker/grafana/dashboards/cipherbox-staging.json`:
- Around line 2061-2115: The panel titled "Kubo Datastore / Memory" is only
querying the memory metric go_memstats_alloc_bytes{job="kubo"} (targets[0].expr)
so it doesn't show datastore usage; either rename the panel title (the "title"
property currently "Kubo Datastore / Memory") to something like "Kubo Memory /
Heap" to reflect the go_memstats metric, or replace/add the target expression in
targets (e.g., update targets[0].expr or add another target) to point to the
actual datastore metric your system exports so the panel truly represents
datastore usage.
---
Outside diff comments:
In `@apps/api/src/republish/republish.processor.ts`:
- Around line 25-49: processRepublishBatch can return processed===failed without
throwing, so batchResult remains "success" today; update the try block after
receiving result (the same place where the code currently logs the "All
republish entries failed" warning) to set batchResult = 'error' when
result.processed > 0 && result.succeeded === 0 && result.failed ===
result.processed so endBatchTimer({ result: batchResult }) reports "error"; keep
existing metricsService.republishEntriesProcessed increments and logger.warn in
place but ensure batchResult is flipped before the finally block calls
endBatchTimer.
---
Duplicate comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 289-350: The finally block calls endTimer using timerResult which
can remain "success" if an exception occurs after the delegated-routing catch
(e.g., folderIpnsRepository.findOne(), BigInt conversions, or
parseIpnsRecordBytes); expand the error handling so any exception in the whole
resolve flow sets timerResult='error' before rethrowing. Wrap the DB/cache
lookup and sequence-number comparisons (calls to folderIpnsRepository.findOne
and the BigInt(...) conversions used to compare sequence numbers) in a try/catch
(or move the delegated-routing try to cover the whole block) that sets
timerResult='error' and then throws the error, ensuring endTimer always records
error on failures; keep references to endTimer, timerResult, timerSource,
folderIpnsRepository.findOne, BigInt, and delegatedRouting.resolve to locate the
changes.
---
Nitpick comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 55-61: The current code uses atob() to decode dto.record into a
string then to bytes (recordBytes), which breaks the end-to-end Uint8Array
requirement; replace the atob-based path with a direct base64-to-bytes
conversion (e.g., use Buffer.from(dto.record, 'base64') or other direct base64
decoder) and assign its Uint8Array result to recordBytes, keeping the existing
try/catch and BadRequestException on failure; update references to atob(),
recordBytes, and dto.record in the method in ipns.service.ts accordingly so
binary data remains a Uint8Array throughout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 658b2c77-1fa1-46ab-a5e2-0c00eba10499
📒 Files selected for processing (5)
.planning/ROADMAP.mdapps/api/src/ipns/ipns.service.tsapps/api/src/republish/republish.processor.tsdocker/grafana/dashboards/cipherbox-staging.jsonscripts/baseline-benchmark.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/baseline-benchmark.sh
- .planning/ROADMAP.md
…itle - Set result='delegated_error' when delegated routing publish fails, so the metric distinguishes partial success from full success - Rename misleading "Kubo Datastore / Memory" panel to "Kubo Heap Memory" since it only queries go_memstats_alloc_bytes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b3fd485daa8f
… and resolve errors - republish.processor: set batchResult='error' when all entries fail (processed === failed), so TEE outages are visible in dashboards - ipns.service resolveRecord: add outer catch to set timerResult='error' for DB/parse failures that bypass the delegated-routing catch block - Update test expectation to match corrected all-entries-failed behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6115dd8c1054
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+2061
to
+2063
| "type": "stat", | ||
| "title": "Kubo Heap Memory", | ||
| "gridPos": { |
There was a problem hiding this comment.
The PR description calls out a Kubo datastore health panel, but the dashboard currently adds a "Kubo Heap Memory" panel instead. Either add a datastore-size panel once the correct Kubo metric name is confirmed, or adjust the panel title/PR description to reflect that this is a fallback proxy and datastore size isn’t currently graphed.
| try { | ||
| await this.delegatedRouting.publish(dto.ipnsName, recordBytes); | ||
| } catch (error) { | ||
| result = 'delegated_error'; |
There was a problem hiding this comment.
ipfsIpnsDuration dashboards/requirements treat result as a small fixed set (e.g., success|error|timeout) and the percentile panels filter result="success". Setting result = 'delegated_error' will exclude these observations from the latency panels and also from the error-rate panel (which filters result="error"). Consider keeping result as success here (since the API still returns success) and tracking delegated routing failures via a separate counter (or update the dashboard queries to include this value).
Suggested change
| result = 'delegated_error'; |
Comment on lines
+306
to
+313
| // Fall back to DB cache on BAD_GATEWAY (delegated routing failures) | ||
| if (error instanceof HttpException && error.getStatus() === HttpStatus.BAD_GATEWAY) { | ||
| this.logger.warn(`Delegated routing failed for ${ipnsName}, falling back to DB cache`); | ||
| timerResult = 'error'; | ||
| } else { | ||
| timerResult = 'error'; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
On delegated routing BAD_GATEWAY, timerResult is set to 'error', but the method can still successfully resolve via the DB cache later and return a CID. That will record a successful resolve as result="error", skewing the error-rate panel and excluding those samples from the result="success" percentile panels. Consider setting result based on the overall outcome (success if either source returns a CID) and using source to indicate DB vs network.
| if (result) { | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
When neither delegated routing nor the DB cache has a record, the method returns null (and the controller converts this into a 404), but timerResult is never changed from its initial 'success'. This will record 404 resolves as result="success" in the histogram. Consider setting timerResult to something like error/not_found before returning null.
Suggested change
| // Neither delegated routing nor DB cache has a record — treat as not_found in metrics | |
| timerResult = 'not_found'; |
| }, | ||
| "targets": [ | ||
| { | ||
| "expr": "libp2p_swarm_connections_opened_total - libp2p_swarm_connections_closed_total", |
There was a problem hiding this comment.
This expression isn’t scoped to the Kubo scrape target (no {job="kubo"} / instance filter), so if any other scraped target ever exports libp2p_swarm_connections_* metrics the panel could mix sources. Consider adding a consistent selector (e.g. {job="kubo"}) like the go_goroutines{job="kubo"} query below.
Suggested change
| "expr": "libp2p_swarm_connections_opened_total - libp2p_swarm_connections_closed_total", | |
| "expr": "libp2p_swarm_connections_opened_total{job=\"kubo\"} - libp2p_swarm_connections_closed_total{job=\"kubo\"}", |
Comment on lines
+2049
to
+2054
| "expr": "rate(libp2p_network_out_bytes_total[$__rate_interval])", | ||
| "legendFormat": "Outbound", | ||
| "refId": "A" | ||
| }, | ||
| { | ||
| "expr": "rate(libp2p_network_in_bytes_total[$__rate_interval])", |
There was a problem hiding this comment.
These bandwidth expressions also aren’t scoped to the Kubo scrape target. Consider adding {job="kubo"} (and optionally grouping by instance) so the panel reliably represents the IPFS node even if other libp2p exporters are added later.
Suggested change
| "expr": "rate(libp2p_network_out_bytes_total[$__rate_interval])", | |
| "legendFormat": "Outbound", | |
| "refId": "A" | |
| }, | |
| { | |
| "expr": "rate(libp2p_network_in_bytes_total[$__rate_interval])", | |
| "expr": "rate(libp2p_network_out_bytes_total{job=\"kubo\"}[$__rate_interval])", | |
| "legendFormat": "Outbound", | |
| "refId": "A" | |
| }, | |
| { | |
| "expr": "rate(libp2p_network_in_bytes_total{job=\"kubo\"}[$__rate_interval])", |
Comment on lines
+33
to
+34
| API_URL="${1:?Usage: $0 <api-url> <jwt-token>}" | ||
| JWT_TOKEN="${2:?Usage: $0 <api-url> <jwt-token>}" |
There was a problem hiding this comment.
This script requires the JWT access token to be passed as a positional CLI argument (<jwt-token>), which will typically be persisted in shell history and visible in process listings, exposing it in cleartext. On multi-user machines or in recorded terminal sessions, an attacker could recover this token from ~/.bash_history or ps output and impersonate the user against the staging API. Prefer reading the token from an environment variable, a local config file excluded from version control, or prompting for it interactively so that it is not stored in shell history or exposed via process arguments.
Closed
Merged
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
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.
Summary
cipherbox_ipfs_ipns_duration_seconds(labels: operation, result, source) andcipherbox_republish_batch_duration_seconds(labels: tee_provider, result) to MetricsServicestartTimer()duration observersipfs:5001/debug/metrics/prometheus)scripts/baseline-benchmark.sh) for reproducible p50/p95/p99 capture against stagingPhase
Phase 18: Performance Instrumentation (requirements PERF-01 through PERF-04)
Test plan
bash -nsyntax check/metricsendpoint emits new histogram metrics./scripts/baseline-benchmark.sh https://api-staging.cipherbox.cc <JWT>to capture baselines🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores