fix(telemetry): address review findings on the shared-metrics sender

Independent review found the claim mechanism did not work. Reproduced
against the real store: two senders POSTed the same package.

The claim wrote next_attempt_at = now, but selection requires
next_attempt_at <= now, so a concurrent pass matched the same row
immediately. It now writes a LEASE INTO THE FUTURE
(_CLAIM_LEASE_SECONDS), which is what actually excludes another pass,
and expires by itself if a process dies mid-send. _mark is additionally
guarded on send_state so a straggler whose lease lapsed cannot
overwrite a completed send back to pending.

The old concurrency test could not fail: it raised AssertionError from
inside a transport, and _send_one catches every exception as a
retryable transport error. It now records what the second pass saw.

Also from review:

- shutdown() never joined the send thread; the join was only wired into
  deactivate(). A short-lived CLI therefore killed an in-flight send at
  exit, on the only cadence this feature has.
- Removed HERMES_TELEMETRY_ENDPOINT. AGENTS.md reserves HERMES_* for
  secrets, and a behavioural override here was a consent hazard: an
  inherited variable could silently redirect telemetry a user agreed to
  send to Nous. The staging E2E writes the endpoint into its throwaway
  profile instead, which also exercises the real config path.
- Added the  shared-metrics toggle that AGENTS.md requires
  as the third opt-in surface, delegating to the setup prompt so the
  consent rules stay in one place.
- Non-429 4xx (401/403/404/413/422) are now permanent. Only 400 was,
  so a wrong path or oversized body retried every 15 minutes for 30
  days until retention pruned it.
- The opt-in day is stamped when the user consents, not on the first
  send pass, which silently dropped the opt-in day whenever the next
  export crossed midnight UTC.
- gzip now uses mtime=0. The embedded timestamp made two sends of one
  package differ on the wire, so the 'byte-identical retry' E2E was
  comparing parsed bodies and could not have caught it. It now compares
  raw request bytes.
- Reconciled the three stale claims in relay-shared-metrics.md that
  said no remote-delivery path exists.

233 tests pass (was 213). Staging E2E re-run through the config path:
both packages 202, and the service logged both objects written to S3.
This commit is contained in:
Ben Barclay
2026-08-26 16:31:52 +10:00
parent 055d58ba33
commit 49757d5e39
12 changed files with 405 additions and 53 deletions
+17 -10
View File
@@ -33,8 +33,12 @@ than downloading a different implementation.
When Relay managed execution is active, the provider request and response pass
through that native module in the Hermes process so configured interceptors can
operate on the real call. This is separate from the shared-metrics data
contract. Shared-metrics mode installs no network exporter and its subscriber
accepts only the versioned, allowlisted projection described below. Enabling a
contract. Shared-metrics mode installs no rich-observability network exporter,
and its subscriber
accepts only the versioned, allowlisted projection described below. The
opt-in package sender described in Appendix A is the only outbound path, it
transmits nothing unless the user enables both `enabled` and `send`, and it
sends whole packages rather than live spans. Enabling a
separately configured rich-observability or dynamic plugin can create a
different data path and requires its own policy review.
@@ -226,17 +230,17 @@ packages from that profile and can therefore link those local packages.
Deleting `$HERMES_HOME/telemetry/shared_metrics` resets the identifier together
with all aggregates and package files.
This slice has no remote-delivery path. A future remote exporter must not reuse
Remote delivery is opt-in and off by default. A remote exporter must not reuse
the persistent local identifier by default. It requires a separate product and
privacy decision covering consent, identity scope, rotation or keyed
pseudonymization, reset behavior, retention, and deletion.
> That exporter is now being built as Phase 2 of the Hermes telemetry project.
> The decisions this paragraph asks for are recorded in
> [Appendix A](#appendix-a-remote-exporter-decisions-phase-2). Until Phase 2
> ships, the statement above still describes shipped behaviour: nothing is
> transmitted, and transmission stays opt-in behind a config key that is off by
> default.
> Those decisions are recorded in
> [Appendix A](#appendix-a-remote-exporter-decisions-phase-2), and the exporter
> implementing them has shipped. Collection alone still transmits nothing: the
> sender runs only when `telemetry.shared_metrics.send` is also true, and it
> transmits a rotating HMAC of the install identity rather than the identifier
> itself.
The install identity is scoped to one `HERMES_HOME`. To reset it, stop Hermes
processes and remove `$HERMES_HOME/telemetry/shared_metrics`. This deliberately
@@ -267,10 +271,13 @@ ID, tool-result, and skill-name canaries are absent from the packages.
## Appendix A: Remote Exporter Decisions (Phase 2)
Status: **decided, not yet built.** This appendix answers the product and
Status: **implemented.** This appendix answers the product and
privacy questions that "Current Slices" defers to a future remote exporter. It
records what was decided and why, so the reasoning survives the implementation.
Sending is off by default and requires both `telemetry.shared_metrics.enabled`
and `telemetry.shared_metrics.send`.
The exporter sends the package files already written under
`$HERMES_HOME/telemetry/shared_metrics/outbox/` to the Hermes telemetry ingest
service. That service validates only the envelope (`schema_version` plus a UUID