Add service.instance.id to mailroom telemetry resource#1745
Draft
DanGould wants to merge 2 commits into
Draft
Conversation
Move the OTLP Resource construction out of the binary's init_tracing_with_telemetry into a library function build_telemetry_resource, gated #[cfg(feature = "telemetry")]. contrib/test.sh only runs --lib and --test integration targets, so any test in main.rs would never run in CI. The extraction puts the construction where it can be tested. Unit test verifies service.name = "payjoin-mailroom" and operator.domain equals the configured value.
Each call to build_telemetry_resource now generates a UUID v4 and adds it as service.instance.id. Grafana Cloud maps this attribute to the Prometheus instance label, giving each process its own series so cumulative counters no longer interleave across nodes. uuid is added to [dependencies] as optional, activated by the telemetry feature. The dev-dependency is kept because the integration test uses uuid unconditionally (not behind a feature gate). Test extended: service.instance.id is present, non-empty, parses as a UUID, and differs across two constructions (verifying per-boot uniqueness). TODO-TESTS.md deleted. Cargo-minimal.lock: uuid 1.18.0 was already pinned as a dev-dep in upstream's lockfile; no new entries are required for the promotion to optional regular dep (verified: cargo check --locked --all-features --all-targets passes against the unmodified upstream lockfile). A from-scratch regen churns ~2700 lines of unrelated environmental drift (crates.io index moved since the file was last committed); the lockfile is therefore left at the upstream-committed pins.
Collaborator
Coverage Report for CI Build 29486348286Coverage increased (+0.04%) to 86.227%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
3 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.
PR body draft — agent/mailroom-otel-instance-id
Title: Add service.instance.id to mailroom telemetry resource
Every mailroom node pushes OTLP metrics with a resource of only
service.name+operator.domain. Grafana Cloud mapsservice.nametojoband
service.instance.idtoinstance. Other resource attributes land only intarget_info. With noservice.instance.id, every node shares identicalseries identity, so cumulative counters from N processes interleave inside
ONE stored series per metric. Prometheus
rate()reads each backward jump asa counter reset and re-adds roughly the full counter value.
Observed in production: 5 nodes reporting (
count(target_info)= 5) but asingle series for the label-less
http_requests_started_total. The dashboardrendered a climbing ~100k req/s sawtooth from a service doing ~5 req/s (the
counter never exceeded ~5.5M — a genuine 100k/s over a 4-minute rate window
would require 24M of increase).
Fix: attach a per-boot
service.instance.id(UUID v4) so each process getsits own series.
Resourceconstruction out ofmain.rsinto thelibrary so it is testable —
contrib/test.shonly runs--liband--test integrationtargets, so a test in the binary would never run in CI.uuidis promoted from dev-dependency to anoptional dependency of the
telemetryfeature; both lockfiles alreadypinned uuid 1.18.0, so there is no lockfile change.
Tests assert
service.name,operator.domain, and thatservice.instance.idis present, non-empty, parses as a UUID, and differsacross constructions. Local gates green: fmt, codespell, clippy
(
--all-featuresand--no-default-features), 65 lib + 3 integration tests,contrib/lint.sh, doc lint (-D warnings),nix fmt -- --ci.Nodes must redeploy to benefit; until all operators update, non-upgraded
senders continue to collide with each other (mitigable stack-side by
promoting the
operator.domainresource attribute onto metrics).Disclosure: co-authored by Claude Code