Skip to content

fix(telemetry): view http.client.request.duration onto 8 buckets - #347

Merged
mrz1836 merged 3 commits into
mainfrom
fix/otelhttp-client-duration-views
Sep 23, 2026
Merged

mrz1836 merged 3 commits into
mainfrom
fix/otelhttp-client-duration-views

Conversation

@oskarszoon

Copy link
Copy Markdown
Contributor

Closes #333.

What

The meter provider in telemetry/telemetry.go registered no Views, so http.client.request.duration — the histogram otelhttp.NewTransport records on all four outbound paths (teranode/client.go, merkleservice/client.go, bump/datahub.go, services/webhook/service.go) — kept whatever bucket set the instrumentation advises. That is 14 boundaries / 15 buckets per attribute set, on a metric already multiplied by target host, method and status code.

Nothing downstream can re-bucket an explicit histogram, so the lever has to exist here.

This registers one View narrowing it to 8 boundaries:

var httpClientDurationBoundaries = []float64{0.005, 0.025, 0.1, 0.25, 1, 2.5, 10, 30}

Nine buckets instead of fifteen.

Correction to the issue

#333 claims a possible unit mismatch — "semconv puts http.client.request.duration in seconds, but the SDK default boundary set is millisecond-shaped, so every bucket above the first may be empty". That is wrong, and there is no correctness bug here. The instrumentation ships its own seconds-shaped advisory, which the SDK honours:

  • otel/semconv/v1.43.0/httpconv/metric.go declares the instrument metric.WithUnit("s") with a default advisory of 0.005 … 10
  • otelhttp/internal/semconv/client.go:41 passes that same seconds-shaped list when constructing the instrument
  • otelhttp/internal/semconv/client.go:336 records durationToSeconds(md.RequestDuration)

So the metric is already in the right unit against the right buckets. The issue's series-count argument stands; its correctness argument does not. The count is also 14 boundaries, not 15.

Why these boundaries

Every value is also an edge of latencyBuckets in metrics/metrics.go, so a quantile taken off this histogram and one taken off arcade_teranode_request_duration_seconds for the same calls interpolate over shared edges rather than two unrelated grids.

The top boundary is 30s rather than the 10s the issue suggested, because defaultTimeout is 30 * time.Second on both teranode/client.go and merkleservice/client.go. With a 10s ceiling a timed-out request and a merely slow one are the same datapoint in the overflow bucket — which is exactly the case worth alerting on.

arcade_* metrics are structurally unaffected

They reach the reader through prometheusbridge.NewMetricProducer. PeriodicReader.Collect runs the view-resolved pipeline and then appends the producer's ScopeMetrics directly, so external producer output bypasses view resolution entirely. Their promauto bucket definitions stay authoritative regardless of how a View is scoped. The View criterion is also the exact, wildcard-free instrument name, which could not match an arcade_* name in any case.

Both facts are asserted by tests rather than just reasoned about.

Deliberately not included

No AttributeFilter. url.scheme, server.port and network.protocol.version look like an 8x multiplier but are functionally determined by server.address for a fixed set of targets — removing them saves bytes per datapoint and zero series. server.address does multiply, but it is the attribute that separates the four outbound paths and is worth its cost.

http.response.status_code is the remaining independent dimension. Unlike bucket boundaries it can be collapsed downstream, so it is left alone here.

Tests

telemetry/telemetry_test.go drives a real otelhttp.NewTransport against an httptest server through a ManualReader configured with the same bridge producer initMetrics uses, then reads the collected metricdata:

  • view_narrows_the_boundary_set — asserts Unit == "s" (unit-drift guard) and that the collected bounds equal httpClientDurationBoundaries. Fails if the View does not reach the instrument otelhttp actually creates.
  • baseline_without_the_view — the control: same setup, no Views, yields exactly the 14-value advisory. This is what makes the assertion above meaningful, and doubles as a drift alarm if a dependency bump ever reverts to the SDK default.
  • TestMetricViews_LeaveBridgedArcadeMetricsAlone — observes on the real metrics.TeranodeRequestDuration and compares the bridged family's bounds against what prometheus.DefaultGatherer reports, read from the registry rather than hardcoded.
$ go build ./...          (exit 0)
$ go vet ./telemetry/...  (exit 0)
$ go test ./telemetry/... -count=1
ok  github.com/bsv-blockchain/arcade/telemetry  0.739s
$ go test ./telemetry/... -race -count=1
ok  github.com/bsv-blockchain/arcade/telemetry  2.146s

All 9 pre-existing tests in the package still pass. gofmt -l telemetry/ is empty. golangci-lint could not run locally — the installed binary is built with go1.26 against this module's go 1.27.0 target — so CI will need to cover it.

Separate bug worth its own issue

http.client.request.body.size and http.client.response.body.size have no advisory (httpconv/metric.go sets only description and unit By). They therefore do fall back to the SDK's default 0, 5, 10, 25 … 10000 — which for a byte-valued histogram means everything above ~10KB lands in the overflow bucket. That is the genuinely-mismatched histogram #333 suspected, just on a different metric, and it is 15 more buckets each. Out of scope here; happy to follow up.

The meter provider registered no Views, so the histogram otelhttp records
on all four outbound paths (teranode, merkleservice, bump/datahub, webhook)
kept whatever bucket set the instrumentation advises: 14 boundaries, 15
buckets per attribute set, on a metric already multiplied by target host,
method and status code. Nothing downstream can re-bucket a histogram, so
the lever has to exist here.

Register one View narrowing it to 8 boundaries. The unit is seconds — the
instrument is declared with unit "s" and the transport records
durationToSeconds — so the boundaries are seconds too, and each is an edge
of the shared latency buckets in metrics/metrics.go so quantiles off this
histogram and off arcade_teranode_request_duration_seconds interpolate over
the same edges. The top boundary is 30s, the client timeout on the teranode
and merkleservice paths, so a timeout is still distinguishable from a slow
success instead of both falling in the overflow bucket.

The arcade_* families are untouched: they reach the reader through the
Prometheus bridge producer, whose ScopeMetrics are appended at collection
time and never pass through view resolution, and the View matches one
wildcard-free instrument name anyway. Tests drive a real otelhttp transport
and assert both the narrowed boundaries and a bridged arcade_* histogram
keeping the buckets its promauto definition declares, with the pre-View
advisory pinned as the control.

Closes #333
@oskarszoon
oskarszoon requested a review from mrz1836 as a code owner September 22, 2026 09:53
Copilot AI lite review requested due to automatic review settings September 22, 2026 09:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the bug-P3 Lowest rated bug, affects nearly none or low-impact label Sep 22, 2026
@github-actions github-actions Bot added the size/L Large change (201-500 lines) label Sep 22, 2026
@oskarszoon
oskarszoon requested a review from galt-tr September 22, 2026 09:56
Copilot AI review requested due to automatic review settings September 22, 2026 12:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mrz1836 mrz1836 assigned galt-tr and unassigned mrz1836 Sep 22, 2026
Copilot AI review requested due to automatic review settings September 22, 2026 16:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mrz1836 mrz1836 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mrz1836
mrz1836 merged commit 4c1b02f into main Sep 23, 2026
32 checks passed
@mrz1836
mrz1836 deleted the fix/otelhttp-client-duration-views branch September 23, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-P3 Lowest rated bug, affects nearly none or low-impact size/L Large change (201-500 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

otelhttp client metrics need SDK Views: 15 default duration buckets, no lever to change them

4 participants