Conversation
Collaborator
Coverage Report for CI Build 35623740801Coverage increased (+0.3%) to 87.037%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
DanGould
force-pushed
the
mailroom-metrics-privacy
branch
from
September 21, 2026 11:40
26fe43d to
7881e8c
Compare
Logging switched to structured JSON whenever a [telemetry] section was present, so the section did two jobs: it named the OTLP endpoint and it chose the log format. An operator who wanted JSON logs had to configure an export, and one who configured an export got JSON logs whether or not a collector was reading them. Add a top-level log_format key, "text" by default or "json", and build the subscriber from it before any telemetry is set up. The section no longer touches logging. Operators who relied on it for JSON logs add log_format = "json" when they upgrade.
Add settled-window gauges for the five counts eligible to leave the operator boundary: completed requests, started requests, mailbox writes, tunnel sheds, and distinct mailbox IDs touched. Each covers the most recently completed Monday-to-Monday UTC week and carries no attributes. Releasing only completed fixed weeks means there is no live value for a prober to watch move and no overlapping windows to difference for daily traffic. The gauges carry exact counts. Rounding to a bin buys nothing: a few units of noise on a counter hides nothing from someone who can send known traffic and subtract it back out. Withholding small counts protects nothing either: a passive viewer would lose only a handful of small values, and the same subtraction recovers them. The unique count is the size of the set of mailbox IDs touched in the settled week. A short ID is already the first eight bytes of a SHA-256, so the set holds the IDs as they are, one set for the week in progress and one for the previous week, older sets dropped at rollover. Each set is capped at ten million entries: past that inserts stop and the week reports the cap, which marks a flooded week rather than counting it. The set replaces the HyperLogLog sketches that approximated the same quantity over hourly, daily, weekly and monthly windows, the unique_short_ids gauge they fed, and the hyperloglogplus dependency. Both lockfiles lose the crate. Nothing wires the gauges to an exporter yet; the precise instruments registered by MetricsService::new are otherwise unchanged.
DanGould
force-pushed
the
mailroom-metrics-privacy
branch
from
September 21, 2026 15:54
7881e8c to
118bf7c
Compare
Route the OTLP export through the settled-window gauges. With a [telemetry] section present, the export provider now carries only the five coarse weekly gauges; the precise per-request counters and live gauges are registered on a provider with no reader and never leave the process. The export provider is no longer installed as the global meter provider, so no instrument can reach it through the global API and bypass the export-surface test. The exported resource is built from an empty resource, not the SDK default, so the environment detector cannot add hostnames or other attributes through OTEL_RESOURCE_ATTRIBUTES. It carries exactly the service name and operator.domain, and a test pins that key set. The operator_domain config key is unchanged: the label is operator-chosen, the operator's public domain is the expected value, and the Foundation's dashboards group by it. The value is an OperatorDomain newtype that cannot be blank, so a section that would write the same unlabelled series as every other blank one fails at config parse and no code path can build a provider under an empty label. Pushes happen hourly instead of every minute. Every push in a week carries the same frozen settled-week value, so pushing more often adds delivery attempts, not resolution. Hourly still lands the value within an hour of a restart or of an outage ending. Omitting the section exports nothing; there is no separate switch.
The four counter buckets behind the exported weekly gauges lived only in memory. A redeploy mid-week zeroed the settled week, and that week reported as empty. Operators redeploy, so the buckets now live in weekly_counts.txt under storage_dir: eight integers keyed by week in a small text format with a version header, loaded at boot, written by renaming a process-specific temporary file into place so a reader never sees a partial file. The buckets are written once per export collection, from the first gauge callback, and again when the process receives SIGINT or SIGTERM. The serve loops now return on those signals instead of dying in the default handler; connections are dropped as before, the only added work is the write. Writing on every request would cost a syscall per request on a busy node for no benefit, since the export only reads the settled week once an hour, and writing only at shutdown would lose the week on a crash. Between the two, a crash costs at most the counts recorded since the last hourly export. A missing file is a fresh install. An unreadable one is logged and ignored rather than refusing to start; the next write replaces it. Two processes sharing a storage directory cannot corrupt the file, because each renames its own temporary file, but the last writer wins and each exports only its own share of the week. One process per operator_domain is the supported deployment. The two sets of mailbox IDs behind unique_short_ids_weekly follow the same rules in a sibling file, unique_short_ids.bin: a version header, then for each week its number, its entry count and its IDs as raw bytes. A separate binary file keeps the counters file readable by eye and its parser a line splitter, and the fixed-width framing makes the ID reader a length check and an 8-byte chunker rather than a text parser over what can be millions of entries. One file for both windows means one rename per write and no listing of storage_dir, which also holds every mailbox file. Up to two weeks of touched mailbox IDs therefore sit on disk, at most ten million per week, next to the mailbox files that already carry those IDs as names.
Describe in the README what leaves an operator's machine once a [telemetry] section is configured: five settled-week counts, exact and including zero, no attributes on the data points, and a resource of exactly the service name and operator.domain, each rule pinned by a test. Say plainly that operator_domain is chosen by the operator and that their public domain is the expected value, that one process per value is the supported deployment, that precise metrics have no local sink in this release, that the unique count is the exact size of a capped set of mailbox IDs and what the cap means, and what a restart or an outage does to the weekly buckets and ID sets. State that up to two weeks of touched mailbox IDs sit on disk under storage_dir. Record that logs are never exported. Update config.example.toml to match, fix the README's link to it, and add the changelog entry.
Once a minute, log one INFO line under the target payjoin_mailroom::heartbeat with the requests in flight, the open bootstrap tunnels, and on Linux the process's open file descriptor count and soft limit. The in-flight and tunnel gauges are OpenTelemetry up-down counters, which can only be pushed to a metrics sink, and the export path sends none of them. A descriptor leak announces itself as long-lived tunnels climbing toward the limit over hours, and the structured log every operator already has is the right place to see that curve. The service keeps atomic copies of both counts, updated in the same calls as the instruments, so the heartbeat reads them without touching the meter. The interval is a constant and the line is not gated on the telemetry feature or config: local logs are always there. The descriptor fields come from /proc/self and are omitted on other platforms.
DanGould
force-pushed
the
mailroom-metrics-privacy
branch
from
September 21, 2026 16:09
118bf7c to
05217e8
Compare
DanGould
marked this pull request as ready for review
September 21, 2026 16:22
Member
Author
|
CI failing because the .#release devShell cannot build & the Docker job fails only because it waits on that one. NixOS/nixpkgs#563137 has the fix |
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
4 participants
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.
Make the mailroom's exported metrics coarse enough that or anyone who sees it cannot follow an individual operator or user's traffic.
Before this change, every node with a
[telemetry]section pushes precise per-request counters and live gauges to Grafana Cloud every minute, labelled with the operator's domain. Operators get no view of those metrics themselves and the only reader is the Foundation's stack. The README did not say what is sent.What the branch does
Read the README diff first; it is the spec. Then the two tests that enforce it (
export_surface_is_windowed_gauges_with_allowed_attributes_onlyinsrc/metrics.rsandexport_resource_carries_only_allowlisted_attributesinsrc/telemetry.rs). The commits add one thing at a time.Two things a reviewer might ask:
Known limits, stated in the README: precise metrics are not observable anywhere while export is on (a local sink is a separate feature TBD); one process per
operator_domain, since two would each write their own share of the week to the same series.Out of scope
_totalcounters to the_weeklygauges. Grafana-side work after merge; theoperator.domainlabel is unchanged so nothing else moves.service.instance.idbranch. This PR pins the resource to two attributes, the opposite design; that branch is parked, not rebased.Disclosure: co-authored by Claude Code