INFOPLAT-13349: feat(beholder): track beholder.export.* metrics per export via custom gRPC stats handler - #2251
INFOPLAT-13349: feat(beholder): track beholder.export.* metrics per export via custom gRPC stats handler#2251kirqz23 wants to merge 1 commit into
Conversation
|
👋 kirqz23, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
Adds a custom metric to restore per-node visibility into OTLP log export volume by capturing the uncompressed outbound gRPC payload size and emitting it as beholder.logs.export.bytes (labelled by csa_public_key).
Changes:
- Introduces a gRPC
stats.Handler(sizeCaptureHandler) to capturestats.OutPayload.Length. - Wraps the OTLP logs exporter with
meteredLogsExporterto increment a bytes counter on successful exports (to avoid retry inflation). - Wires the stats handler + metered exporter into
NewGRPCClient’s shared log exporter connection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/beholder/metered_exporter.go | Adds size-capture stats handler and metered exporter wrapper emitting beholder.logs.export.bytes. |
| pkg/beholder/client.go | Wires the shared capture + exporter wrapper and attaches the gRPC stats handler(s) to the log exporter dial options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dialOpts := []grpc.DialOption{ | ||
| grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelOpts...)), | ||
| grpc.WithStatsHandler(&sizeCaptureHandler{capture: capture}), | ||
| } |
There was a problem hiding this comment.
gRPC builds istats.NewCombinedHandler(...) from the registered slice of multiple handlers, that is exactly the delegating handler proposed by the Copilot. This proposal is exactly what gRPC already builds for us internally. The downside of having multiple stats handlers however is that gRPC fires every handler on every event, in registration order. So for one OutPayload event there will be two HandleRPC(...) calls which might be an overhead. We might consider either keeping both statsHandlers if we need them, or dropping grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelOpts...)).
This old handler:
- Emits
rpc.client.*metrics (which part of them are going to be removed), - Creates a trace span per RPC
- Injects trace-context headers
There was a problem hiding this comment.
So, we have two approaches here:
- Keep two stats handlers as we have it now.
exportSizeHandlershouldn't add much to the performance. - Drop
grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelOpts...))taking into account that we loose 3 points mentioned in the above comment, howeverrpc.client.*metrics are totally deprecated anyway expect duration, which can be added to our custom handler alongsidebeholder.export.bytes, e.g. sth likebeholder.export.duration
cc @pkcll
…RPC stats handler
1440cca to
1fec9f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/beholder/client.go:580
- In
newMeterProvider, the previous implementation appendedcfg.metricOptions()(which includessdkmetric.WithCardinalityLimit(cfg.MetricCardinalityLimit)) but the new code constructs the MeterProvider without it. This silently drops the configured per-instrument cardinality limit, which can increase time-series cardinality and memory usage in production.
return sdkmetric.NewMeterProvider(
sdkmetric.WithReader(sdkmetric.NewPeriodicReader(metered, readerOpts...)),
sdkmetric.WithResource(resource),
sdkmetric.WithView(cfg.MetricViews...),
), metered, nil
pkg/beholder/client.go:153
- The PR description states
beholder.export.*should be labelled per signal type (logs/metrics/traces), but the implementation only meters log and metric exports. Trace exports created innewTracerProviderare not wrapped and do not useexportSizeHandler, sobeholder.export.bytes/beholder.export.durationwon't be emitted forotel_signal="traces".
// Shared export instruments beholder.export.bytes and
// beholder.export.duration, labelled per signal. They live on this
// MeterProvider, so the metrics exporter can only be wired up once the
// provider and its meter exist.
expMetrics, err := newExportMetrics(meter)
What
Adds a
beholder.export.bytesandbeholder.export.durationmetrics to the beholder gRPC client, restoringper-node log volume visibility and export duration (labelled by
csa_public_keyandsignal_typelogs/metrics/traces)According to these docs,
rpc.client.request.*metrics are totally deprecated without any successors in otelgrpc semconv v1.40.0RPC semantic convention stability migration guide
Semantic conventions for RPC metrics
Additionally,
beholder.export.*metrics are not recorded on separate messages, but on the whole batch at once. In terms ofbeholder.export.bytesit shows real number of bytes exported on success without cumulative number on all retries thatrpc.client.request.sizewas producing.Changes
exportSizeHandler(gRPCstats.Handler) to capture outbound message sizes viaOutPayloadeventsbeholder.export.byteswith attributesotel_signalandcsa_public_keybeholder.export.duration(histogram, units) with attributesotel_signal,csa_public_keyanderror0.005–60); the SDK defaults are millisecond-scaled, so nearly every export would land in the first bucketexportMetricsstruct, attached to the metric exporter once theMeterProviderexists (attachMetrics)error={true,false}Notes
HandleRPCbecause the uncompressed proto length is only observable inside the gRPC stack. Duration is measured in the wrapper instead, so it also covers proto marshaling, connection setup and inter-attempt backoff — and is still recorded when an export fails before any RPC is issued (e.g. an already-expired context, which produces no stats events at all).Requires
Supports