Repository navigation
[INFOPLAT-19425] Send legacy telemetry via a dedicated chip-ingress batch client - #23949
skippaDaBitFlippa wants to merge 9 commits into
Conversation
…atch client With TelemetryIngress.ChipIngressEnabled, the telemetry manager now uses an injected chip-ingress service instead of Beholder's chip client, which was a no-op unless Telemetry.ChipIngressEndpoint was also set. - Add TelemetryIngress.ChipIngressEndpoint and ChipIngressInsecureConnection, validated when chip ingress is enabled. - Build a dedicated chipingress.Client with CSA auth and NOP lookup. - Replace the per-(contract, type) workers with one shared chainlink-common batch client; Close now flushes. - Emit OTI-parity CloudEvent extensions (legacytelemetry, csapublickey, partitionkey, sentat in ns, receivedat in ms); stop sending empty nodeoperatorname/nodename/nodecsapublickey. - Count telemetry_client_messages_sent/send_errors per message. - Record telemetry_ingress.config.info.
|
✅ No conflicts with other open PRs targeting |
- Map telemetry type to CloudEvent domain/entity only in the batch client; drop Domain/Entity from TelemPayload and ChipIngressAgent. - Manager: drop the per-endpoint chip service field (endpoints are only an allowlist in chip mode), share endpoint construction, and simplify the config-info metric. - Batch client: inline the health ping, drop the redundant datacontenttype, and share a shouldLogCount helper with the WSRPC clients. - Chip client: use the global OTel providers Beholder installs. - Tests: generate the ChipIngressService mock with mockery, drop wiring and library-behaviour tests, and fold duplicated setup and metric helpers.
- Split Send into newEvent, onPublished and drop so it reads top to bottom; build failures now share the rate-limited drop log. - Document what the client is for and which Kafka header each CloudEvent extension becomes. - Name the health ping interval/timeout and the endpoint metric label; rename the receiver to c. - Rewrite the NewManager and telemetryEndpoint doc comments.
Add TelemetryIngress.ChipIngressBufferSize (10000), ChipIngressMaxBatchSize (1000), ChipIngressMaxConcurrentSends (10) and ChipIngressDrainTimeout (5s), validated > 0 when chip ingress is enabled. Defaults match the previous hard-coded values and Beholder's chip batch defaults. SendInterval and SendTimeout continue to drive the batch interval and per-RPC timeout. NewChipIngressBatchClient now takes a ChipIngressBatchConfig.
| if auth != nil { // nil only when there are no headers and no TTL | ||
| opts = append(opts, chipingress.WithTokenAuth(auth)) | ||
| } | ||
| opts = append(opts, chipingress.WithNOPLookup()) |
There was a problem hiding this comment.
This is quite important server side, maybe we should add a comment like:
// Load-bearing: legacy telemetry identity headers on chip-ingress thin
// topics (node-operator-name, node-name) are derived server-side from
// the NOP info this option requests (x-include-nop-info). Removing it
// silently blanks those headers downstream
There was a problem hiding this comment.
Added in 1f61d2f. The comment above WithNOPLookup() says chip-ingress derives ce_nodeoperatorname/node-operator-name and ce_nodename/node-name from the NOP info that x-include-nop-info requests (nopInfoHeadersFromContext in chip-ingress write_service.go), and that the node never sends them itself, so removing the option silently blanks those headers.
There was a problem hiding this comment.
@pkcll it's odd to me that we have this at all, I feel like we should be fetching this NOP info regardless of this header. the single auth call to csa-auth should be enough fetch everything
we should maybe revisit this behavior and retire this header
@thomaska at the very least we should have chip-ingress fetch this when in legacy telem mode regardless if the header is set
With the pinned chainlink-common batch client, Stop flushes only the batch being filled; messages queued behind in-flight sends are discarded without a callback (found in local load testing). Correct the Close and ChipIngressDrainTimeout docs. Fix upstream: smartcontractkit/chainlink-common#2461.
pkcll
left a comment
There was a problem hiding this comment.
Review: legacy telemetry via chip-ingress batch client
With TelemetryIngress.ChipIngressEnabled=true, the node builds a dedicated CSA-authenticated chip-ingress client and one shared chainlink-common batch client. Every telemetry agent sends through that client as CloudEvents carrying the legacy-telemetry extensions plus x-include-nop-info. The per-(contract, type) chip workers are gone, and the flag-off WSRPC path is unchanged.
The happy path follows the atlas#13712 contract closely, and Send is non-blocking and bounded. The problems are in two areas:
- The config surface becomes a public contract once released, because
DecodeTOMLrejects unknown fields, so keys are hard to rename or remove later. Today it defaults to the prod endpoint, misses bounds that can panic or abort boot, reuses WSRPC timing keys, and can hard-exit shutdown. - Failure behavior with the flag on: there is no capability check or back-off, batch splitting is quadratic, LLO nodes see a WARN flood, and there is no per-type isolation.
Requesting changes to the config surface before merge; code-path items block the first flag flip, not the merge. Merging with the flag off is safe. Nothing below changes flag-off behavior beyond the small deltas listed under Backwards compatibility.
Before merge (config surface)
- Remove the prod default for
ChipIngressEndpoint, require it when enabled, and tag the changeset#changed(M1). - Add bounds and
host:portsyntax checks to validation, and run them whether or not the flag is on (M2). - Add chip-specific
ChipIngressSendInterval,ChipIngressSendTimeoutandChipIngressMaxGRPCRequestSize, mirroring[Telemetry], instead of reusing the WSRPC keys (M3). - Default
ChipIngressDrainTimeoutto1sand cross-check it againstShutdownGracePeriod(M4). - Stop sending
receivedat, and pin the exact extension set in a test (M5).
Before the first flag flip
- Add a capability check: gate publishing on the already-authenticated Ping once it reports the server mode, and add a breaker with back-off on permanent errors (M6; needs a small atlas#13712 change).
- Fix the O(n²) split upstream, or default
ChipIngressMaxBatchSizeto ≤ 100, sized from measured per-node rates (M7). - Count unsupported types quietly and use a time-based log limiter (M8).
- Meet the server prerequisites in atlas#13712 (see Rollout plan).
Findings
There are no Critical findings, and nothing below can crash or stall a node with the flag off.
- M1–M5 block merge.
- M6–M8 block the first flag flip.
- M9 should land before ramping past the canary cohort.
- M10 should be decided before the prod canary.
Major
M1. The default endpoint is the production hostname, and an existing flag changes meaning.
- Where: core/config/docs/core.toml:120.
- What changed: before this PR,
ChipIngressEnabled=trueused Beholder's chip client. That client is aNoopClientunlessTelemetry.ChipIngressEndpointis set, so the flag effectively did nothing; the changeset itself says telemetry was "silently dropped". Now the same TOML sends real traffic tolegacy-telemetry.prod.telemetry.chain.link:443, and flag-on with all defaults passes validation. - Impact: any staging, CRIB or testnet node that already sets the flag starts shipping to prod on upgrade.
- What I could check: no system-test or CRIB config in this repo sets it. Infra and NOP configs aren't visible to me.
- Fix:
- Default the endpoint to
''. The existingMissingErrorthen forces an explicit value. - Tag the changeset
#changedand describe the new meaning. - Confirm with infra whether any fleet sets the flag today.
- Default the endpoint to
M2. ValidateConfig misses bounds and syntax, and is skipped entirely when the flag is off.
- Where: core/config/toml/types.go:1048-1071.
- Early return when the flag is off (L1049-L1051):
ChipIngressMaxBatchSize=0andChipIngressEndpoint=''both pass with the flag off.- Bad staged values only surface at flip time, as a node that won't boot, during the riskiest step of the rollout.
- No upper bounds:
- TOML decode already rejects negatives and values above MaxInt64, so the
uint→intcast can't wrap. ChipIngressBufferSize = 9223372036854775807validates, then panics inNewApplicationwithmakechan: size out of range.ChipIngressBufferSize = 1e8validates and allocates about 754 MiB at boot (measured locally).ChipIngressMaxConcurrentSendsandChipIngressMaxBatchSizeare unbounded.
- TOML decode already rejects negatives and values above MaxInt64, so the
- Endpoint is only checked for non-empty:
- An endpoint with no port, or with a scheme (
https://…,dns:///…), passes validation. NewApplicationthen fails (application.go:553-555) after keystore unlock.chainlink node validatenever sees it.
- An endpoint with no port, or with a scheme (
- Fix:
- Run range and syntax checks always. The checked keys are new in this PR and all have defaults, so this costs nothing.
- Gate only the "required when enabled" checks on the flag.
- Bounds are in the validation table below.
- Add
node validatetestdata for a chip-enabled config.
M3. Chip batching reuses the WSRPC SendInterval/SendTimeout and diverges from the chip knobs already in [Telemetry].
- Where: application.go:556-564. Compare with the
[Telemetry]chip knobs at core.toml:906-918, which are validated at types.go:3226-3246. SendTimeout='0s'is valid. EveryPublishBatchthen runs on an already-expired context, so all telemetry is silently dropped (measured locally).SendInterval='0s'sends one RPC per message.- A value tuned for WSRPC becomes the chip batching delay. For example, the full-config fixture's
'1m0s'(config-full.toml:43) would mean a one-minute delay. - There is no request-size key, but the library's 10 MiB default must stay within the server's receive limit.
- Fix:
- Add
ChipIngressSendInterval,ChipIngressSendTimeoutandChipIngressMaxGRPCRequestSizewith the same names and meaning as in[Telemetry]. - Share one bounded
validateChipBatch(...)helper between the two sections. - Stop reading WSRPC keys in chip mode.
- Add
M4. A hung telemetry server turns every node shutdown into a hard exit.
- Where:
ChipIngressDrainTimeout='5s'(core.toml:130) equalsShutdownGracePeriod='5s'(core.toml:19). When the grace period runs out, the node callsos.Exit(-1)(core/cmd/shell_local.go:411-419). - How it happens:
- The library (
chainlink-common/pkg/chipingress@v0.0.11-0.20260915184316-2730f1867c92,batch/client.go) runs in-flightPublishBatchcalls oncontext.WithoutCancel. - So
Stopwaits the full drain timeout when the server hangs. Measured locally:Closetook exactlyDrainTimeout. - Services close in reverse order (application.go:1099-1103). Heartbeat, the P2P wrapper, the durable emitter, CRE services, the audit logger and nurse close after telemetry, so their
Closeis skipped. - WSRPC doesn't have this problem: it cancels on Close and never drains.
- The library (
- Fix:
- Default to
1s. - Reject
ChipIngressDrainTimeout >= ShutdownGracePeriod/2inCore.ValidateConfig(types.go:128), which already does cross-section checks. - Or drop the drain entirely, since delivery is at-most-once anyway.
- Default to
M5. receivedat couples every node to a server admission rule that could cause 100% loss.
- Where: core/services/synchronization/chip_ingress_batch_client.go:190.
- Today it is harmless but misleading. The server overlays its own
received-at, yet a node-clockce_receivedatsurvives on every record. - The risk is in admission rules:
- If atlas#13712 legacy admission is ever tightened to reject server-owned or unknown extensions rather than strip them, every chip-mode node's events fail
VALIDATION_FAILED. - The same applies to the extensions the library stamps on its own:
recordedtime(fromNewEvent) andseqnum(fromQueueMessage).
- If atlas#13712 legacy admission is ever tightened to reject server-owned or unknown extensions rather than strip them, every chip-mode node's events fail
- Fix:
- Delete line 190.
- Add a test that pins the exact extension set on the wire, including the library-stamped ones.
- Agree in atlas#13712 to strip, not reject, server-owned extensions, at least until no released node sends them.
M6. There is no capability check, back-off or fallback, and a wrong-mode server is invisible to the node.
- Where: Ping at chip_ingress_batch_client.go:91-104, callbacks at 132-150, and chip mode replacing WSRPC at core/services/telemetry/manager.go:111-119.
- Failure handling:
- Any
PublishBatchfailure is counted, sample-logged and the batch dropped. - There is no retry, back-off or state change; the next tick sends again, indefinitely.
- Nothing falls back to WSRPC. Telemetry stays lost until someone restarts the node with the flag off.
- Any
- What the node can and can't see:
- Ping goes through the server's CSA auth interceptor. An unreachable server or a rejected key therefore does show as
connection_status=0plus a health error. - The mode is not visible. A reachable chip-ingress with legacy mode off, or a non-legacy deployment, answers
"pong"and rejects every event individually. - So
connection_statusstays1while 100% of events fail. The only signals are an error counter with no reason (m2) and sampled logs.
- Ping goes through the server's CSA auth interceptor. An unreachable server or a rejected key therefore does show as
- Fix:
- Capability check. The server returns its mode in
PingResponse.message(today a constant"pong"). The node publishes only while the mode islegacy-telemetry. Otherwise it logs one ERROR, sets a gauge and re-probes every 30 s, without failing boot. As a bonus, a server-sidepausedmode gives a kill switch that needs no restart. - Breaker in a node-owned decorator around
chipingress.Client(see m1). Open after N consecutive batches with a permanent code:Unimplemented,PermissionDenied,Unauthenticated,InvalidArgument, or all eventsVALIDATION_FAILED. - While open, drop immediately, back off with jitter from 5 s to 5 min, and probe with one batch.
- Expose
telemetry_client_chip_circuit_open{reason}. - Treat
Internalas transient (see Rollout plan).
- Capability check. The server returns its mode in
M7. The default ChipIngressMaxBatchSize = 1000 hits an O(n²) split in the pinned library.
- Where: core.toml:126. In the library (
chainlink-common/pkg/chipingress@v0.0.11-0.20260915184316-2730f1867c92,batch/client.go),splitMessagesByRequestSizere-runsproto.Sizeover the growing candidate slice for every message. - Measured locally (2 KB payloads): 7 ms per batch at 100 events, 80 ms at 250, 250 ms at 500, 646 ms at 1000, and 2.58 s at 2000.
- CPU model: CPU ≈ 0.65 µs × rate × B once batches fill. Batches fill when rate ×
SendInterval≥ B, or while draining a backlog after an outage.- B=1000 at 1000 msg/s costs about 0.65 core.
- B=250 at 5000 msg/s still costs about 0.8 core, so 200–250 is not a fix on its own.
- Why a canary won't catch it: under normal load, batches are small and interval-bound. The cost appears when the server recovers and the buffer drains, taking CPU from OCR/LLO.
- Fix:
- Preferred: accumulate per-event sizes upstream (
protowire.SizeTag(1) + protowire.SizeBytes(proto.Size(event))), which makes the split O(n). Beholder's emitter, also at batch size 1000, pays the same cost today. - Until that lands: default to ≤ 100, size it from per-CSA-key rates taken from OTI, and cap it in validation.
- Preferred: accumulate per-event sizes upstream (
M8. On LLO nodes, unsupported types cause a per-message WARN flood and a drop metric that never returns to zero.
- Where:
- chip_ingress_batch_client.go:118-128 and 154-163.
llo-reporthas no mapping (common.go:104-105).- LLO sends it whenever
CaptureReportTelemetryis on (llo/telem/telemetry.go:252-253).
- What happens:
- Every
llo-reportmessage fails innewEventand callsdrop(). SendresetsdropCounton every successful enqueue (L127), andllo-reportinterleaves with the observation and outcome types. The 1, 2, 4… throttle therefore restarts constantly. Measured locally: 1000 interleaved drops produced 1000 WARN lines.- Report telemetry is sampled to about one per channel per second, so that is roughly one WARN per channel per second, indefinitely.
messages_dropped{endpoint="chip-ingress"}mixes "unsupported by design" with "buffer full", so it can't be used for alerting.errorCountnever resets either, so error logging scales with message rate.
- Every
- Fix:
- Check support once per type and count unsupported types in
telemetry_client_messages_unsupported_total{telemetry_type}. - Replace both throttles with a time-based limiter: one line per type per 30 s, carrying a suppressed count.
- Add a test with interleaved types.
- Check support once per type and count unsupported types in
- Data parity is unaffected (OTI drops these types too).
M9. A single shared FIFO removes per-type isolation.
- Where: chip_ingress_batch_client.go:118-128 plus the library's single
messageBuffer. - WSRPC, like the old chip path, has one queue per (contract, type), so a noisy type only fills its own queue.
- Here one high-volume type (LLO observation, enhanced EA, head reports) can fill all 10 000 slots and starve OCR2/OCR3 reports. Measured locally: after an
llo-observationflood, 10/10ocr2-medianmessages were dropped. - Not regressions:
- Drop-newest is also WSRPC's policy (telemetry_ingress_batch_client.go:212-219).
- A 10k total is below WSRPC's aggregate (100 per worker), so memory is no worse.
- Fix: a per-type admission quota in
Send(e.g. ≤ X% ofBufferSize, using an atomic counter decremented in the callback) or a few lanes.
M10. There is no shadow mode, and the kill switch needs a restart.
- Where: manager.go:111-119.
- Each node uses either WSRPC or chip, never both. Paths can't be compared per node.
- Rollback means a config edit plus a restart. A binary downgrade also fails on the new keys.
- Fix: one of
- the fallback transport in Design (automatic, no duplicates), or
ChipIngressMode = off | shadow | only, with the server writing shadow traffic to non-prod topics.
- Without either, cohort comparison is the only parity evidence, and the first prod canary must be an internal node.
Minor
m1. Delivery semantics are inherited from the library without being pinned.
- The retry JSON in
chainlink-common/pkg/chipingress@v0.0.11-0.20260915184316-2730f1867c92(client.go) is not wrapped inmethodConfig/retryPolicy, so gRPC ignores it. Measured locally: 1 attempt, versus 3 for a correctly wrapped control config. - So a 1 s
Unavailableblip drops up toMaxBatchSizemessages. At-most-once is fine for telemetry, but please document it. - The risk: once upstream fixes the JSON, a routine bump starts retrying
RESOURCE_EXHAUSTEDthree times against an overloaded server. - Discard-on-stop, the split (M7),
seqnumstamping and partial delivery are also unpinned library behavior. - Fix: one node-owned decorator that owns retry explicitly and hosts the M6 breaker, plus tests that pin these behaviors.
m2. The send-error metric has no reason. telemetry_client_messages_send_errors (L135) can't tell failure modes apart. batch.ErrorCodeFor(err) is bounded but only appears in sampled logs. Add telemetry_client_chip_errors_total{error_code}.
m3. Ping failures add a new /health signal.
- Where: chip_ingress_batch_client.go:97-100 calls
EmitHealthErron every failed Ping (every 5 s). - WSRPC only sets a gauge (telemetry_ingress_batch_client.go:159-180) and emits a health error only on unexpected dial errors.
- So every telemetry-server blip now turns
/healthinto a 207 (health_controller.go:83-94)./readyzis unaffected. - Fix: use the gauge only, or emit the health error only after a sustained outage.
m4. Close discards queued messages without counting them (L106-L113). This is documented, but the loss never shows up in messages_dropped, so every restart drops data invisibly. Track an atomic in-flight count and add it to a shutdown_discarded counter.
m5. Unknown chains and unmapped job types become Noop agents, with an ERROR per job.
- Chains unknown to
chain-selectorsfail at chip_ingress.go:51-54. WSRPC sent telemetry for every configured chain. - Unmapped job types fail at chip_ingress.go:57-59. That covers vault-plugin (ocr2/delegate.go:837), DKG, DON-time, ring, and generic plugins (1587).
- Data parity holds, since OTI drops unknown types too.
- Fix: one startup summary, a
telemetry_ingress_chip_noop_agents{network}gauge, and a per-chain check during rollout.
m6. Chip mode still requires the OTI URL/ServerPubKey (manager.go:162-168), even though newEndpoint returns early in chip mode. If those fields are removed, the chain silently drops off the allowlist. In chip mode, require only Network and ChainID.
m7. The chip client is built even with no [[TelemetryIngress.Endpoints]] (application.go:544). It dials and pings every 5 s but sends nothing. Skip construction and log a WARN.
m8. The node keeps a routing table the server ignores.
- The node sends real
source/typedomains (common.go:67-107). The comment at L166 says they select the topic, but the legacy server routes ontelemetrytype. - Against a chip-ingress build without legacy code, those domains go through the normal pipeline instead, and are either dropped or written to real domain topics.
- Fix: in legacy mode, send a constant non-routable
source/type(e.g.legacy-telemetry/legacy.Telemetry) and keep only a set of supported types on the node. Old servers then fail closed. Confirm with the server owners first.
m9. Flag-off behavior is not byte-identical.
- A start hook plus an hourly goroutine publish
telemetry_ingress.config.infoeven with the flag off (manager.go:76-79, 196-210). - Exported surface changes:
NewManager's signature changed;Manager.MonitoringEndpointGeneratoris removed;TelemPayloadlostDomain/Entity;config.TelemetryIngressgained six methods, which breaks out-of-tree implementers.
- All harmless, but worth one line in the changeset.
m10. The added complexity lands in the two largest constructors. Against the merge base, NewApplication goes from cyclomatic 70 to 73 and cognitive 100 to 105, and NewManager from cognitive 7 to 11 (gocyclo v0.6.0, gocognit v1.2.1). The extraction in Design addresses both.
Nitpicks
TelemPayload.ChainIDis never populated (chip_ingress.go:116-122).onPublishedcaptures the whole payload, including theTelemetrybytes, but needs onlyTelemTypeandContractID.
Backwards compatibility
Flag off.
- The WSRPC clients are built exactly as today (manager.go:187-193).
- The new keys are optional with defaults, old configs parse, and the config and docs tests pass.
- The only deltas are m9, plus M2: invalid chip values staged while the flag is off go unnoticed.
Binary downgrade.
- Older binaries reject configs that contain the new keys, because
DecodeTOMLusesDisallowUnknownFields(chainlink-commonpkg/config/toml.go). - Keep rollback as a flag flip on the same binary for at least one release, or remove the keys before downgrading.
Flag on, by server state:
| Server state | Node behavior | Visible to node? |
|---|---|---|
| Unreachable, DNS or TLS failure | Ping fails: connection_status=0 plus a health error. Batches fail fast and are dropped. When the buffer is full, the newest messages are dropped |
Yes; no back-off (M6) |
| CSA key not registered or bad token | Ping is authenticated, so same as above | Yes |
| Reachable, legacy mode off | Ping returns "pong". Every event is rejected individually while the RPC returns OK |
No, only the error counter and logs (M6) |
| Older chip-ingress without legacy code | The marker is ignored, and events route by source/type through the normal pipeline |
Only if dropped; m8 makes this fail closed |
Unimplemented, PermissionDenied |
Every batch fails, on every tick | Yes; needs the breaker (M6) |
| Request too large | The library splits at 10 MiB. If the server limit is lower, the request is rejected every time | Yes; make the size configurable (M3) |
Internal |
The whole batch is counted as failed | Yes, but without a reason |
| Server NOP lookup fails | The event is written without identity headers, and the RPC returns OK | No, server metric only |
| Mixed fleet (some nodes WSRPC, some chip) | Both paths write the same legacy topics with identical keys. Each node uses one path, so there are no duplicates | n/a |
PublishBatchTelemetry status
- It does not exist. There is no
PublishBatchTelemetrysymbol or RPC in chainlink, chainlink-common, chainlink-protos or atlas. The PR calls the existingPublishBatchRPC throughchainlink-common/pkg/chipingress/batch.Client. - Client side: done. All options used (
WithEventClone,WithShutdownTimeout,WithClientName,ErrorCodeFor,WithNOPLookup) exist in the already-pinnedchipingress v0.0.11-0.20260915184316-2730f1867c92.go.modbumps neither chainlink-common nor chainlink-protos. - Server side (atlas#13712): legacy mode lives inside the existing publish path: marker gate, validation, routing by
telemetrytype, header overlay and partitioner. It is enabled per deployment withENABLE_LEGACY_TELEMETRY, and there is no new RPC. - Implications:
- A server without legacy mode accepts the call instead of returning
Unimplemented. The only way to detect it is to ask, which is why the mode belongs in Ping (M6). - The real dependency is the server mode. It must be live before any node flips the flag.
- Older servers accept the events and route them by
source/type(m8). - Retries, splitting and discard-on-stop are library behavior and can change with a bump (m1).
- Still open: CSA-principal authorization for the legacy topics, and the server's receive limit compared with the node's request size.
- A server without legacy mode accepts the call instead of returning
Design: transport as a plugin, configured once
Current state.
- Good: the chip client is resolved at the composition root, and the hot path has no flag checks.
- But the manager branches on
chipService != nilin six places (L58, 82, 111, 136, 182, 207). - WSRPC, the default, is inline code in
newEndpointrather than an implementation of an interface. - Runtime got simpler: a fixed goroutine set replaces per-(contract, type) workers. Code got flatter only on the surface, because complexity moved into the library (m1) and the constructors (m10).
Proposal. One interface with two implementations, chosen once at the root. The manager only checks the allowlist and delegates.
// core/services/telemetry/transport.go
type Chain struct{ Network, ChainID string } // upper-cased
// Transport turns an allowlisted chain + job into a monitoring endpoint. Exactly one is chosen at the root.
type Transport interface {
services.Service
Agent(c Chain, contractID string, t synchronization.TelemetryType) (commontypes.MonitoringEndpoint, error)
MultitypeAgent(c Chain, contractID string) (MultitypeMonitoringEndpoint, error)
}
func (m *Manager) GenMonitoringEndpoint(network, chainID, contractID string, typ synchronization.TelemetryType) commontypes.MonitoringEndpoint {
c := NewChain(network, chainID)
if _, ok := m.allowed[c]; !ok {
m.eng.Warnf("no telemetry endpoint for %v; telemetry %q for %q will NOT be sent", c, typ, contractID)
return &NoopAgent{}
}
a, err := m.transport.Agent(c, contractID, typ)
if err != nil {
m.eng.Warnw("telemetry disabled for job", "chain", c, "type", typ, "err", err)
return &NoopAgent{}
}
return a
}// core/services/chainlink/telemetry.go: extracted from NewApplication. Boot-safe by construction.
func newTelemetryTransport(cfg config.TelemetryIngress, auth chipAuth, ks keystore.CSA, lggr logger.Logger) telemetry.Transport {
if !cfg.ChipIngressEnabled() {
return telemetry.NewWSRPCTransport(telemetry.WSRPCConfigFrom(cfg), ks, lggr) // default, unchanged
}
t, err := telemetry.NewChipTransport(telemetry.ChipConfigFrom(cfg), auth, lggr) // all knobs resolved once
if err != nil {
// Structural errors were rejected by ValidateConfig; anything left is environmental.
lggr.Errorw("chip-ingress telemetry disabled; node continues without legacy telemetry", "err", err)
return telemetry.NoopTransport{}
}
return t
}wsrpcTransportholds today'snewEndpointbody.URL/ServerPubKeyare required only there, which fixes m6.chipTransportis the single home for:- the supported-type check (M8);
- the capability gate and breaker (M6);
- the client decorator (m1).
NewManager(allowed []Chain, t Transport, lggr)drops all six nil checks, andNewApplicationloses its 28-line block.- Optional, for rollout: a
fallbackTransportthat builds both transports and checks one atomicchipReadyper message.chipReadyis set by the capability check and breaker, so it is runtime state, not config. Each message takes exactly one path, which gives fallback to WSRPC without a restart and without duplicates (M10).
This doesn't block merge. I'd still do it now, because it removes the branches and gives M6 and M8 a single place to live.
Config validation and boot safety
Boot-safety rule:
- Decidable from the TOML alone → reject in
ValidateConfig. This covers types, ranges,host:portsyntax and cross-field checks. The node refuses to boot like it would for any invalid TOML, andnode validateand CI catch it. These checks run even with the feature off. - Needs I/O or external state → never fails boot. This covers DNS, TLS, auth, server mode and keystore contents. The transport starts degraded, logs one ERROR, sets a gauge and keeps probing.
- Construction error after validation passed → fail open. Log an ERROR and use a Noop transport. Never
return nil, errfromNewApplicationbecause of telemetry.
| Invalid config | Current behavior | Proposed |
|---|---|---|
| Flag on, endpoint unset | Valid; sends to prod | Reject: no default (M1) |
| Endpoint with no port, a scheme, or whitespace | Valid; NewApplication fails, or DNS fails at runtime |
Reject: trim, net.SplitHostPort, no scheme, port 1–65535 |
ChipIngressInsecureConnection=true with a public host |
Valid; the CSA token is sent in clear | Reject for *.chain.link; otherwise log an ERROR |
ChipIngressBufferSize = MaxInt64 or 1e8 |
Valid; boot panic, or about 754 MiB at boot | Reject: [1, 100000] |
ChipIngressMaxBatchSize = 1e6 |
Valid; quadratic split (M7) | Reject: [1, 500] |
ChipIngressMaxConcurrentSends = 1e9 |
Valid; unbounded senders, and Stop loops over every slot |
Reject: [1, 32] |
SendTimeout='0s' |
Valid; every RPC expires, total silent loss | Reject: ChipIngressSendTimeout in [1s, 60s] (M3) |
SendInterval='0s' or '1h' |
Valid; one RPC per message, or an hour of delay | Reject: ChipIngressSendInterval in [50ms, 10s] |
ChipIngressDrainTimeout ≥ ShutdownGracePeriod (defaults: 5 s = 5 s) |
Valid; hard exit when the server hangs (M4) | Reject in Core.ValidateConfig |
Telemetry.AuthHeadersTTL in (0, 10m) |
Valid; client construction fails at boot | Reject in Telemetry.ValidateConfig |
| Any of the above with the flag off | Valid (early return) | Reject, always (M2) |
Flag on, no Endpoints |
Valid; dials, sends nothing | Log a WARN and skip construction (m7) |
Flag on, Endpoints without URL/ServerPubKey |
Valid; the chain silently drops off | Don't require them in chip mode (m6) |
Chain unknown to chain-selectors |
Noop agent per job | Runtime only, plus a summary and metric (m5) |
Server down, TLS, Unauthenticated, mode off, Unimplemented |
Per-batch errors, no back-off | Runtime only: capability check and breaker (M6) |
| Construction error after validation passed | Node doesn't boot | Log + disable (Noop transport) |
Node safety knobs
Send never blocks the caller: building an event costs 3.3 µs, 71 allocs and about 4.6 KB per message (measured locally), which is negligible. Goroutines and metric labels are bounded, and there are no retry storms. The risk sits in the knobs:
| Knob | Default | Validated? | Recommended |
|---|---|---|---|
ChipIngressEndpoint |
prod hostname | non-empty, only when enabled | no default; host:port, always |
ChipIngressInsecureConnection |
false | no | reject for public hosts |
ChipIngressBufferSize |
10000 | > 0, only when enabled |
keep; [1, 100000] |
ChipIngressMaxBatchSize |
1000 | > 0, only when enabled |
≤ 100 until the O(n) split; [1, 500] |
ChipIngressMaxConcurrentSends |
10 | > 0, only when enabled |
4; [1, 32] |
SendInterval (shared with WSRPC) |
500ms | no | ChipIngressSendInterval, [50ms, 10s] |
SendTimeout (shared with WSRPC) |
10s | no | ChipIngressSendTimeout, [1s, 60s] |
ChipIngressDrainTimeout |
5s | > 0, only when enabled |
1s; < ShutdownGracePeriod/2 |
| Max request size (not exposed) | 10 MiB (library) | n/a | ChipIngressMaxGRPCRequestSize ≤ server receive limit |
Telemetry.AuthHeadersTTL (shared) |
0 (static token) | no | 0 or ≥ 10m; decide whether to require rotation (Q8) |
Rollout plan
The order is server first, then the node with the flag off, then canaries, then ramp. Pause whenever a gate fails.
Server prerequisites (atlas#13712) that change node outcomes:
- Network mappings.
network-mappings.jsonis missing about 30 OTI-enabled prod networks, 13 of them mainnets (e.g.megaeth-mainnet,pharos-mainnet,robinhood-mainnet,tempo-mainnet). Those nodes' telemetry would be rejected at cutover. The node flag is global per node, so these chains can't be excluded individually. - Missing topics. One event routed to a missing topic fails the whole
PublishBatchwithInternal. The healthy records are still written, but the node counts them as failed. - Strict-mode rejections are returned as
Internalwith the reason lost, so the node's breaker must treatInternalas transient. - Also needed before the first flip: mode in Ping (M6), and strip-not-reject for server-owned extensions (M5).
- Merge with the flag off. Land the before-merge items and release with
ChipIngressEnabled=false.- Exit: one release cycle with no WSRPC metric change,
config.info{chip_ingress_enabled="false"}present, and no CPU/RSS regression.
- Exit: one release cycle with no WSRPC metric change,
- Server. Deploy the dedicated legacy deployment (
ENABLE_LEGACY_TELEMETRY=true) to staging, then prod. Never on a shared chip-ingress.- Close the gaps above.
- Verify the legacy topics and partition counts, schemas, CSA auth and NOP lookup, CSA-principal topic authorization, and that the receive limit is at least the node's request size.
- Check keepalive: the client pings every 10 s with no active stream. A
too_many_pingsGOAWAY would drop in-flight batches, because there is no retry. - Keep the hostname unresolvable, or pointed only at this deployment, until the checks pass.
- Exit: synthetic smoke tests pass with a test CSA key. Positive: all types, including head-report with an empty contract id. Negative: a mode-off deployment, a missing extension, a wrong
csapublickey/partitionkey. Identity headers present.
- Staging canary, after the flip-blockers land: one internal node with an explicit staging endpoint for at least 48 h. Include an LLO node with report telemetry on, and one node with a high job count.
- Watch
splitMessagesByRequestSizeCPU share, WARN rate,/health207s, and any "Shutdown grace period … exceeded" lines.
- Watch
- Prod canary. Start with one internal node, then 1–3 cooperative NOP nodes, for at least 72 h including a busy period. Include a chain that was fixed in the mappings.
- Compare each node against its own 7-day OTI baseline, per (CSA key, type, network), on the Kafka side.
- Node counters don't compare across paths: WSRPC
messages_sentcounts batches, chip counts messages.
- Ramp. 5% → 25% → 50% → 100% of NOPs, at least 24 h per step. Land M9 before going past the canary cohort.
- Gate each step on server RPS, produce latency and consumer lag.
- Size for about 2 req/s per lightly loaded node, with up to
MaxConcurrentSendsin flight when the server is slow.
- Retire OTI WSRPC after 2–4 stable weeks. Keep the flag as the rollback switch for one more release.
Exit criteria (24 h windows):
- Delivery
sent / (sent + dropped + send_errors)≥ 99.5% per node, excluding unsupported types. - Zero
missing_nop_info. - Per-key hourly counts within ±2% of baseline, with unchanged partition placement.
- Node CPU < 1% of a core at median load, RSS < 50 MB, and no breaker opens.
received-at − sent-atp99 below an agreed bound.- No telemetry-caused shutdown hard exits or
/healthflaps.
Kill switches:
- Server: Ping returns
paused, and nodes stop within one probe interval (needs M6). - DNS: pull the name. Nodes fail fast, and the breaker pauses them.
- Node: flip the flag and restart. With the fallback transport, no restart is needed.
- Downgrade: remove the new keys first.
- Loss on rollback: up to
BufferSizequeued messages, plus in-flight batches, plus the restart gap. There are no duplicates.
Tests run
All at 1f61d2fda368bce5559480ec155f836488828f82:
| Command | Result |
|---|---|
go build ./core/services/telemetry/... ./core/services/synchronization/... |
OK |
go test -race -count=1 ./core/services/synchronization/... ./core/services/telemetry/... |
PASS (including TestChipIngressClient_SendAuthHeaders) |
go test -race -count=5 -run TestChipIngressBatchClient ./core/services/synchronization/ |
PASS, no flakes |
go test -race -count=1 ./core/config/... |
PASS |
go test -race -count=1 -run 'TestConfig|TestTelemetry|TestCoreDefaults|TestDoc' ./core/services/chainlink/ |
PASS |
go test -race -count=1 -run TestResolver_ConfigV2 ./core/web/resolver/ |
PASS |
The "measured locally" numbers above come from throwaway probes against the pinned library on an Apple M3 Max. None of that code is part of this PR.
Not run:
- The
TestScriptsgoldens (node/validate,config). Please confirm them in CI. - The rotating-token auth path (
AuthHeadersTTL > 0). It has no test; the existing test covers only the static token andx-include-nop-infoon Ping.
Worth adding:
- buffer full;
- per-event
PublishError; - whole-batch errors by code;
- shutdown with a hung RPC;
- the exact extension set (M5);
- interleaved unsupported types (M8);
- validation with the flag off;
- the chip wiring in
NewApplication.
Questions
- Is
ChipIngressEnabled=trueset anywhere today (infra, CRIB, NOP configs)? See M1. - Can atlas#13712 return its mode, plus a
pausedstate, inPingResponse.message? See M6. - Are node CSA principals authorized for the legacy topics/domains on the dedicated deployment?
- Do you agree that telemetry construction errors should fail open instead of aborting boot?
- Is the fallback transport or a shadow mode in scope for this PR, or a follow-up? See M10.
- What are typical and maximum LLO/OCR3 payload sizes? They're needed for a byte bound (M9).
- Who owns the O(n) split fix in chainlink-common? Beholder's emitter benefits too.
- Is a static, non-expiring token (
AuthHeadersTTL=0) acceptable for this endpoint? - Can we get per-CSA-key message rates from OTI to size
ChipIngressMaxBatchSize?
What's good
- A non-blocking
Sendand one bounded queue replace an unbounded set of per-contract workers. That is a real runtime simplification. - The contract is followed carefully:
- all required extensions are present;
partitionkeyhandles head-report's empty contract id;sentatis now unix ns, fixing the old RFC3339;- the CSA key matches the authenticated key;
WithNOPLookupis documented as load-bearing (chip_ingress_client.go:43-47) and tested.
- The flag is resolved at the composition root, with no per-message flag checks.
- Per-message counting is noted in the changeset. Shutdown is bounded, and the discard caveat is documented.
- Docs, testdata and mocks are consistent, the flag-off path is intact, and the tests pass under
-race.
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
- Remove the default ChipIngressEndpoint; it is required when the flag is on. - Validate chip settings even with the flag off: bounds, host:port syntax, no insecure connection to chain.link hosts, and DrainTimeout under half of ShutdownGracePeriod. - Add chip-specific SendInterval, SendTimeout and MaxGRPCRequestSize. - Lower defaults: MaxBatchSize 100, DrainTimeout 1s. - Stop sending receivedat; chip-ingress owns it. - Count unsupported telemetry types separately from drops and rate-limit repeated warnings by time. - Mark the service unhealthy only after a sustained ping outage. - Chip mode endpoints need only Network and ChainID; skip the chip client when no endpoints are configured. - Remove the unused TelemPayload.ChainID and the changeset.
|
Hey @pkcll, pushed 2b9a66e with the config surface items and the small fixes:
m1 and m4 were already covered by the I also removed the changeset. For m9, the API changes are M6, M9, M10, the transport refactor, m2 and m5 will come in follow-up PRs before the first flag flip. One thing for atlas#13712 @thomaska: with |
|
I see you updated files related to
|
|
| # ChipIngressEnabled sends all telemetry to ChipIngressEndpoint instead of the WSRPC Endpoints. Endpoints entries are still required and select which chains send telemetry; in this mode only their Network and ChainID are used. | ||
| ChipIngressEnabled = false # Default | ||
| # ChipIngressEndpoint is the host:port of the chip-ingress legacy telemetry endpoint. Required when ChipIngressEnabled is true. Separate from Telemetry.ChipIngressEndpoint, which is Beholder's endpoint. | ||
| ChipIngressEndpoint = '' # Default |
There was a problem hiding this comment.
perhaps we update this default with the production endpoint (once it's provisioned) so this isn't config that needs to be set by NOPs
| telemType := string(payload.TelemType) | ||
| domain, entity, err := TelemetryTypeToDomainAndEntity(payload.TelemType) | ||
| if err != nil { | ||
| // Expected on LLO nodes (llo-report); OTI drops these types too. |
There was a problem hiding this comment.
it's good to capture this error case, though to clarify the comment here, if this is a telemetry type that's not supported, I would expect it to NOT get wired up in the first place else we're just wasting cycles
| err = c.batchClient.QueueMessage(event, c.onPublished(payload.TelemType, payload.ContractID)) | ||
| } | ||
| if err != nil { | ||
| TelemetryClientMessagesDropped.WithLabelValues(chipIngressEndpointLabel, telemType).Inc() |
There was a problem hiding this comment.
AFAIK these metrics are the plain prometheus metrics
we should consider upgrading some/all of these metrics we're setting in this file to Beholder/OTel (perhaps in addition to Prom) so we have enough client visibility on our backends
There was a problem hiding this comment.
yes, we should use OTel in addition to prom metrics
| event.SetExtension("networkname", payload.Network) // endpoint network, e.g. "EVM" | ||
| event.SetExtension("contractid", payload.ContractID) // "contract-address" | ||
| event.SetExtension("csapublickey", c.csaPubKeyHex) // "node-operator-csa-public-key" | ||
| event.SetExtension("partitionkey", payload.ContractID+"-"+c.csaPubKeyHex) // Kafka record key |
There was a problem hiding this comment.
these are technically fine to include, but most of these are going to largely be ignored on the ingress side:
networknameisn't useful for us, we use different names internally (chainselectoris the source of truth here)csapublickeyfine to include, but we should ignore this on the ingress side as well and use the value derived from the actual authentication context as that's authoritativepartitionkeywe're ignoring this value on the ingress side and explicitly setting the partition key to be equivalent to what we have in OTI
| } | ||
| event, err := c.newEvent(payload, domain, entity, time.Now()) | ||
| if err == nil { | ||
| err = c.batchClient.QueueMessage(event, c.onPublished(payload.TelemType, payload.ContractID)) |
There was a problem hiding this comment.
just a general note -- it's good we're using the built-in batch client here and not rolling our own, but we need to make sure there's no crucial differences between how we're sending legacy telemetry today and how this batch client sends things
how things get queued, how backpressure is handled, how retries are handled (i.e. we don't want retries here, this is supposed to be fire-and-forget), what happens when some messages fail in a batch but not all of them, etc.
There was a problem hiding this comment.
what happens when some messages fail in a batch but not all of them,
thats partial delivery feature. You get result per event with error per event in response
| TelemetryClientWorkers.WithLabelValues(chipIngress, string(payload.TelemType)).Inc() | ||
| // onPublished returns the callback the batch client runs once the message's batch | ||
| // has been published, counting each message as sent or failed. | ||
| func (c *chipIngressBatchClient) onPublished(telemType TelemetryType, contractID string) func(error) { |
There was a problem hiding this comment.
similar question as before ... this function looks like it's called per message, so what happens if one message fails in a batch (maybe due to a missing topic, for example) but all of the other messages go through?
will all of the messages in that batch be marked as errored?
if so, we'll be seeing a lot of false errors on the metric that gets incremented here
There was a problem hiding this comment.
will all of the messages in that batch be marked as errored?
if partial delivery feature is enabled (default:true) it will deliver all other messages except those with missing topic or schema etc.
| // A non-nil chipService puts the manager in chip mode: every allowlisted chain sends | ||
| // through that one shared service. With nil, each endpoint gets its own WSRPC client. |
There was a problem hiding this comment.
perhaps we need to spend a few agent cycles to make sure this is fine
the comment is highlighting a natural tension here -- this code is originally written to be essentially one WSRPC client per network because that is our OTI deployment model, but in the chip-ingress world we have one endpoint for everything
There was a problem hiding this comment.
We start with 1 batch client 1 buffer 1 connection, observe and decide if we need to optimize
pkcll
left a comment
There was a problem hiding this comment.
PR Review (round 3): #23949 — Send legacy telemetry via a dedicated chip-ingress batch client
Re-review at 2b9a66e8. The previous review (#pullrequestreview-5538325915) was at 1f61d2fda3. Two commits landed since then:
e7e0cebfmerges develop and bumpspkg/chipingresstob866ad79, which adds drain-on-Stop and makes retries opt-in.2b9a66e8addresses the review feedback.
Summary
All five before-merge items (M1–M5) are fixed, and so are M7, M8, m3, m6, m7 and both nitpicks. I checked each one against the code. The bumped library also resolves m1 (retries) and m4 (shutdown loss accounting). The new code adds no Critical or Major findings.
Recommendation: Approve with minor changes (merge with the flag off). The Request changes on the config surface is resolved. What's left before merge is small: restore the changeset, pin partial delivery, and a few nits.
The first flag flip stays gated on the follow-ups the author committed to (M6, M9, M10, m2, m5, transport refactor). m8 is missing from that list. atlas#13712 also changed since our review, and the changes affect what the node sees (see Server-side changes).
Jira Context
INFOPLAT-19425 (node), paired with atlas#13712 / INFOPLAT-19409 (server, still open, head 827a8bfa).
Files Reviewed
The delta 1f61d2fda3..2b9a66e8, excluding the develop merge:
core/config/toml/types.go,core/config/docs/core.tomlcore/services/chainlink/{application.go, config_telemetry_ingress.go, config_test.go}core/services/synchronization/{chip_ingress_batch_client.go, *_test.go, metrics.go, common.go}core/services/telemetry/{manager.go, manager_test.go}- testdata and goldens
Library: chainlink-common/pkg/chipingress@v0.0.11-0.20261015170356-b866ad79d5fd (batch/client.go, client.go, retry_policy.go). Server: atlas#13712 at 827a8bfa (legacy_telemetry.go, write_service.go, kafka/client.go, docs/api.md).
Status of previous findings
| ID | Ask | Status at 2b9a66e8 |
|---|---|---|
| M1 | No prod default endpoint | ✅ Default is '' (core.toml:120) and the endpoint is required when the flag is on. The changeset was removed instead of retagged #changed (see R3-1) |
| M2 | Bounds and syntax, always validated | ✅ inRange/durationInRange cover every chip knob, plus host:port and a port range, with the flag on or off (types.go:1087-1127). Tested |
| M3 | Chip-specific interval, timeout and request size | ✅ ChipIngressSendInterval [50ms, 10s], ChipIngressSendTimeout [1s, 1m], ChipIngressMaxGRPCRequestSize [1, 10] MiB. WSRPC keys are back to WSRPC only. The 10 MiB cap matches the chip-ingress default SERVER_MAX_RECV_MESSAGE_SIZE_BYTES |
| M4 | Drain 1s, checked against grace period | ✅ Default 1s. Rejected when >= ShutdownGracePeriod/2, with the flag on only (types.go:146-154). That is a deliberate, documented exception to M2 so that existing short-grace configs keep booting; I'm fine with it. Stop is now bounded by DrainTimeout, then closes the client (library batch/client.go:217-257) |
| M5 | Drop receivedat, pin the extension set |
✅ The test pins the exact set, including the library-stamped seqnum and recordedtime. The server stamps received-at itself (see Server-side changes) |
| M6 | Capability check and breaker | ⏳ Deferred to a follow-up before the first flip |
| M7 | O(n²) split | ✅ Mitigated: default 100, cap 500. The split is still O(n²) in b866ad79 (splitMessagesByRequestSize), so the upstream fix is still open (Q7) |
| M8 | Unsupported types: quiet counter and time limiter | ✅ telemetry_client_messages_unsupported, Debug level, one line per key per 30s with a suppressed count. The interleaved-types test is added |
| M9 | Per-type isolation | ⏳ Deferred |
| M10 | Shadow mode or fallback | ⏳ Deferred |
| m1 | Pin delivery semantics | ◐ Retries are opt-in in b866ad79 and the node doesn't opt in, so nothing retries ✅. Partial delivery is still an implicit library default (R3-2) |
| m2 | error_code on send errors |
⏳ Deferred |
| m3 | /health flaps |
✅ The gauge drops on every failure. SetHealthCond fires after 12 consecutive failures (about 60s) and clears on the next success. Tested |
| m4 | Count shutdown loss | ✅ With the new library, messages left after DrainTimeout fail against the closed client and count as send_errors. Their callbacks may run after Close returns, but they only touch counters and the logger |
| m5 | Noop-agent summary and metric | ⏳ Deferred |
| m6 | Chip mode needs only Network/ChainID | ✅ (manager.go:172-184). The URL and ServerPubKey getters are nil-safe. Tested |
| m7 | No endpoints: skip construction | ✅ WARN and no chip client (application.go:547-548) |
| m8 | Non-routable source/type in legacy mode | ❓ Not in the author's follow-up list. Only the comment was updated ("source/type only matter to a non-legacy deployment"). Please confirm it is tracked or explicitly dropped |
| m9 | Note API changes in the changeset | ❌ Listed in a PR comment instead, and the changeset was deleted (R3-1) |
| m10 | Constructor complexity | ⏳ Tied to the transport refactor (deferred) |
| Nits | ChainID, onPublished capture |
✅ Both fixed |
Findings
Critical
None.
Major
None in the new code. The deferred flip-blockers (M6, M9, M10, m8) still apply to the first flag flip, not to the merge.
Minor
R3-1. The changeset was deleted.
- Where:
.changeset/telemetry-chip-ingress-endpoint.mdwas removed in2b9a66e8. The changeset bot asked for one again on 10-15. - Why it matters: the PR adds nine public TOML keys under
[TelemetryIngress], whichDecodeTOMLmakes hard to remove later. It also changes exported API: theNewManagersignature, the removedMonitoringEndpointGenerator,TelemPayloadfields, and newconfig.TelemetryIngressmethods. - Fix: restore it tagged
#wip(which holds external communication until the feature is ready), or#internal. Paste the API list from the PR comment into it.
R3-2. Partial delivery is an implicit library default, and no test pins it. This is 4of9's question at chip_ingress_batch_client.go:159.
- Today's behavior is correct. The node doesn't set
WithTransactionEnabled, so it gets the library default (false, partial delivery). Callbacks are then per event, fromPublishResponse.Results(librarybatch/client.go:408-453). - Server side: since atlas
405ca32f06, a failed produce (e.g. a missing topic) fails only its own event. - Measured with a throwaway probe against the PR code:
- One batch of 6 events, every second one with a per-event error, counted 3
sentand 3send_errors. - An OK response with an empty
Resultscounts every event as a send error (RESULTS_MISMATCH). That is fine against atlas, which always fillsResults.
- One batch of 6 events, every second one with a per-event error, counted 3
- The risk: the library default could change in a bump. In strict mode the server returns
Internalfor the whole request, and every healthy event would then count as an error. - Fix:
- Pass
batch.WithTransactionEnabled(false)explicitly, with a comment. - Add a
"partial"case toTestChipIngressBatchClient_CountsPerMessage: an OK response where one result has anErrorshould givesent = n-1andsend_errors = 1.
- Pass
R3-3. Two telemetry construction errors can still stop the node from booting with the flag on.
- Where:
return nil, fmt.Errorf(...)at application.go:559 and L573. - Endpoint syntax is now rejected up front, so the remaining structural case is
Telemetry.AuthHeadersTTLin (0, 10m). It still passes validation, and thenNewHeaderProviderfails at boot. - Fix: reject that range in
Telemetry.ValidateConfig. Separately, Q4 (fail open with a Noop transport when telemetry construction fails) is still unanswered; it fits naturally into the transport refactor.
R3-4. The chip path can reorder telemetry that WSRPC kept in order. This is part of 4of9's question at L147.
- WSRPC runs one worker per (contract, type), and each worker sends synchronously on its own tick (telemetry_ingress_batch_worker.go:59-73). So records for one Kafka key arrived in order.
- The chip path allows up to
ChipIngressMaxConcurrentSends = 10batches in flight. When server latency exceedsChipIngressSendInterval(500ms), or while a backlog drains, a later batch can be produced before an earlier one to the same partition. - Question: does any OTI consumer depend on per-key order rather than
sent-at? If yes, defaultChipIngressMaxConcurrentSendsto 1–2 and size it from Q9's per-node rates.
Nitpicks
- The
chain.linkinsecure guard is case-sensitive (types.go:1123).LEGACY.CHAIN.LINK:443or a trailing-dot host gets past it. Normalize first withh := strings.ToLower(strings.TrimSuffix(host, ".")). - Endpoint syntax gaps (types.go:1113):
- Only leading and trailing whitespace is caught, so
ho st:443passes. host:443/pathis rejected with "port must be between 1 and 65535", which is misleading.- Fix: reject
strings.ContainsAny(endpoint, " \t/")with the syntax message.
- Only leading and trailing whitespace is caught, so
- The test helper doesn't use the shipped defaults (helpers_test.go:32-44).
NewTestChipIngressBatchClientusesMaxBatchSize: 1_000, which validation now rejects, andDrainTimeout: 5s. Use the defaults (100, 1s) so tests run the shipped shape. - The unsupported-type path does extra work per message (chip_ingress_batch_client.go:136-143). Every LLO
llo-reportmessage allocates"unsupported:"+telemTypeand takes the client-widelogLimitermutex just to maybe emit a Debug line. Logging once per type (async.Mapof seen types) is enough there. This also answers most of 4of9's "wasting cycles" at L138, below. - The extension set can shrink (4of9 at L196). Server-side,
csapublickeyandpartitionkeyare ignored in legacy mode; atlasdocs/api.mdsays the node "may stop sending both". Nothing readsnetworkname. Dropping the three saves about 200 B per event and removes extensions that look like identity claims but are never trusted. The server ignores them rather than rejecting them, so this is safe in either order. Update the pinned set inTestChipIngressBatchClient_Event.
Server-side changes since our review (atlas#13712 at 827a8bfa)
These change rows of the round-1 server-state table and the rollout plan:
| Topic | Before (as reviewed) | Now |
|---|---|---|
| Missing topic in a batch | Whole PublishBatch failed with Internal; the node counted healthy events as failed |
Per event in partial-delivery mode (405ca32f06); healthy events are produced and reported as produced. The "Missing topics" rollout prerequisite is resolved |
NOP lookup fails or x-include-nop-info absent |
Record written without identity headers | Event rejected with missing_nop_info (c315319ef8). A node whose CSA key isn't in the NOP registry has its events 100% rejected while Ping stays green, which makes M6 and m2 more important |
received-at |
Server overlay | Still stamped at receipt (legacy_telemetry.go:148-151); it was already in legacy.go at the commit we reviewed. ce_receivedat only existed because the node sent it, and OTI never had it, so no OTI consumer can depend on it |
csapublickey / partitionkey extensions |
Used for key and header | Ignored; key and identity come only from the authenticated CSA key |
On the author's note to thomaska ("my local chip-ingress run didn't stamp received-at"): the legacy path does stamp it, so the event most likely didn't take that path. Check ENABLE_LEGACY_TELEMETRY=true and that the event was counted as accepted legacy telemetry (not legacy_validation_error or missing_nop_info).
Replies to 4of9's inline threads
-
chip_ingress_client.go:47, NOP info header. Agree. Now that atlas requires NOP info and drops events without it, a client header is load-bearing for 100% of legacy data. In legacy mode the server should fetch NOP info for the authenticated CSA key regardless of the header. KeepWithNOPLookup()and its test on the node until that server change is deployed everywhere; it's harmless afterwards. -
core.toml:120, default to the prod endpoint. Fine as the end state. Set the default in the release that ships the M6 capability gate, once Q1 (does any fleet already setChipIngressEnabled=true?) is answered. Until then, no default means a node with the flag on can't silently start sending to prod. -
chip_ingress_batch_client.go:138, unsupported types shouldn't be wired.- Single-type endpoints already get a
NoopAgentat wiring time (chip_ingress.go:56-59). - The
Send-time check only fires for multitype endpoints, where the type arrives with each call; LLO'sSendTypedLog(llo-report)is one. - The check itself is a
switch. The only real waste is the limiter lock and string allocation, covered in the unsupported-type nitpick above.
- Single-type endpoints already get a
-
chip_ingress_batch_client.go:150, Prometheus vs OTel.- The library already emits OTel
chip_ingress.batch.*(requests, sizes, latency, splits, result mismatches) withclient_name=legacy_telemetry. That goes through the global meter provider Beholder installs, so there is request-level backend visibility today. - The per-message outcome counters (
sent,dropped,unsupported,send_errors) are Prometheus-only, and NOP Prometheus isn't visible to us. - Suggestion: dual-emit those four as OTel counters via
beholder.GetMeter(), with attributestelemetry_type(about 20 values) anderror_code(about 10 values), in the m2 follow-up. Keep the Prometheus names for NOP dashboards and WSRPC parity. - Caveat: with Beholder off the OTel provider is a no-op, which is why both are needed.
- This makes the rollout exit criterion "delivery ≥ 99.5% per node" measurable without NOP cooperation.
- The library already emits OTel
-
chip_ingress_batch_client.go:147, differences from today's WSRPC path:Aspect WSRPC (today) Chip batch client Queueing One channel per (contract, type), BufferSize100 eachOne shared channel, 10 000 (M9) Backpressure Drop newest, counted droppedDrop newest, counted dropped; unsupported types counted separatelyRetries None None (opt-in in b866ad79; the node doesn't opt in)Partial batch failure Whole batch fails Per event (R3-2) Ordering per key Preserved (one request in flight per worker) Not guaranteed with concurrent sends (R3-4) Shutdown Cancel, no drain Drain up to DrainTimeout(1s); the rest counts assend_errorsCounting sentandsend_errorscount batchesCount messages -
manager.go:55, one WSRPC client per network vs one endpoint. This is what the transport refactor from round 1 addresses, and the author deferred it. In chip mode,[[Endpoints]]is only a chain allowlist; m6 now makes that explicit.
Questions
- m8: is it tracked in a follow-up, or explicitly dropped now that atlas routes on
telemetrytype? - R3-4: does any consumer of the legacy topics depend on per-key order?
- Can the follow-ups (M6, M9, M10, m2, m5, m8, transport refactor) be listed as INFOPLAT-19425 sub-tasks, so the flag-flip change can reference them as done?
- Carried over from round 1, still open: Q1 (flag set anywhere today), Q4 (fail open), Q7 (O(n) split owner), Q9 (per-CSA-key rates).
Tests run
All at 2b9a66e8677c39c444261cd1d3767fb099d7a9bc:
| Command | Result |
|---|---|
go build ./core/... |
OK |
go vet on synchronization, telemetry, config/toml, chainlink |
OK |
go test -race -count=1 ./core/services/synchronization/... ./core/services/telemetry/... ./core/config/... |
PASS |
go test -race -count=3 -run 'TestChipIngressBatchClient|TestLogLimiter' ./core/services/synchronization/ |
PASS, no flakes |
go test -race -count=1 -run 'TestConfig|TestTelemetry|TestCoreDefaults|TestDoc' ./core/services/chainlink/ |
PASS |
go test -race -count=1 -run TestResolver_ConfigV2 ./core/web/resolver/ |
PASS |
go test -count=1 -run 'TestScripts/node/validate' . and TestScripts/config |
PASS (12 validate goldens, 3 config) |
| Throwaway probe (partial results and empty results; not part of the PR, deleted after running) | 3/3 split on a 6-event batch; empty Results gives all errors |
CI: every check on 2b9a66e8 is pass or skipped, including Core Tests, race, fuzz, GolangCI Lint and the CRE E2E suites.
Positive Notes
- Every config-surface item was handled the way it was asked, including the subtle ones:
- validation runs with the flag off;
- the drain is cross-checked against the grace period;
- chip-specific timing keys replace the shared WSRPC ones;
- the exact extension set is pinned.
- The health change uses
SetHealthCond/ClearHealthCondcorrectly, and the test drives both directions. logLimiterreplaces two counter-based throttles that never reset. Its keys are bounded (type × error code), and the suppressed count makes the lines useful.- The PR comment maps each finding to what changed, with a clear list of what's deferred.
Recommendation
Approve with minor changes for merging with ChipIngressEnabled=false:
- Before merge: R3-1 (changeset) and R3-2 (pin partial delivery and test it).
- Before merge or in a follow-up: R3-3, R3-4 and the nitpicks.
Do not flip the flag on any node until M6, M9 (before ramp), m2 and m8 land and Q1 is answered. M6 and m2 matter more now that the server rejects events whose NOP lookup fails.
|
Follow-up to round 3: send with @skippaDaBitFlippa, one more item for the flip-blocker list. It also answers @4of9's fire-and-forget point at What. Pass batch.WithTransactionEnabled(false), // partial delivery: per-event results (R3-2)
batch.WithNoAck(), // PublishBatchNoAck: server produces with acks=0; legacy telemetry is loss-tolerantWhy
What changes on the node
Rollout prerequisites (please add these to the flip gate)
Timing. Don't hold this PR for it: it is merge-safe with the flag off as it is. Make the switch in the M6 follow-up, since both touch the same client, and before the first flip. |




Send legacy telemetry to chip-ingress through a dedicated CSA-authenticated client and one shared batch client when TelemetryIngress.ChipIngressEnabled is set.