Skip to content

[INFOPLAT-19425] Send legacy telemetry via a dedicated chip-ingress batch client - #23949

Draft
skippaDaBitFlippa wants to merge 9 commits into
developfrom
INFOPLAT-19425-core-node-send-telem-chip
Draft

skippaDaBitFlippa wants to merge 9 commits into
developfrom
INFOPLAT-19425-core-node-send-telem-chip

Conversation

@skippaDaBitFlippa

Copy link
Copy Markdown

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

…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.
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

✅ No conflicts with other open PRs targeting develop

@trunk-io

trunk-io Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

- 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())

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.

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 

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@4of9 4of9 Oct 16, 2026 •

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.

@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.
@thomaska thomaska added the build-publish Build and Publish image to SDLC label Oct 14, 2026

@pkcll pkcll left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 DecodeTOML rejects 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)

  1. Remove the prod default for ChipIngressEndpoint, require it when enabled, and tag the changeset #changed (M1).
  2. Add bounds and host:port syntax checks to validation, and run them whether or not the flag is on (M2).
  3. Add chip-specific ChipIngressSendInterval, ChipIngressSendTimeout and ChipIngressMaxGRPCRequestSize, mirroring [Telemetry], instead of reusing the WSRPC keys (M3).
  4. Default ChipIngressDrainTimeout to 1s and cross-check it against ShutdownGracePeriod (M4).
  5. Stop sending receivedat, and pin the exact extension set in a test (M5).

Before the first flag flip

  1. 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).
  2. Fix the O(n²) split upstream, or default ChipIngressMaxBatchSize to ≤ 100, sized from measured per-node rates (M7).
  3. Count unsupported types quietly and use a time-based log limiter (M8).
  4. 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=true used Beholder's chip client. That client is a NoopClient unless Telemetry.ChipIngressEndpoint is set, so the flag effectively did nothing; the changeset itself says telemetry was "silently dropped". Now the same TOML sends real traffic to legacy-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 existing MissingError then forces an explicit value.
    • Tag the changeset #changed and describe the new meaning.
    • Confirm with infra whether any fleet sets the flag today.

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=0 and ChipIngressEndpoint='' 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→int cast can't wrap.
    • ChipIngressBufferSize = 9223372036854775807 validates, then panics in NewApplication with makechan: size out of range.
    • ChipIngressBufferSize = 1e8 validates and allocates about 754 MiB at boot (measured locally).
    • ChipIngressMaxConcurrentSends and ChipIngressMaxBatchSize are unbounded.
  • Endpoint is only checked for non-empty:
    • An endpoint with no port, or with a scheme (https://…, dns:///…), passes validation.
    • NewApplication then fails (application.go:553-555) after keystore unlock.
    • chainlink node validate never sees it.
  • 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 validate testdata 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. Every PublishBatch then 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, ChipIngressSendTimeout and ChipIngressMaxGRPCRequestSize with the same names and meaning as in [Telemetry].
    • Share one bounded validateChipBatch(...) helper between the two sections.
    • Stop reading WSRPC keys in chip mode.

M4. A hung telemetry server turns every node shutdown into a hard exit.

  • Where: ChipIngressDrainTimeout='5s' (core.toml:130) equals ShutdownGracePeriod='5s' (core.toml:19). When the grace period runs out, the node calls os.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-flight PublishBatch calls on context.WithoutCancel.
    • So Stop waits the full drain timeout when the server hangs. Measured locally: Close took exactly DrainTimeout.
    • 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 Close is skipped.
    • WSRPC doesn't have this problem: it cancels on Close and never drains.
  • Fix:
    • Default to 1s.
    • Reject ChipIngressDrainTimeout >= ShutdownGracePeriod/2 in Core.ValidateConfig (types.go:128), which already does cross-section checks.
    • Or drop the drain entirely, since delivery is at-most-once anyway.

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-clock ce_receivedat survives 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 (from NewEvent) and seqnum (from QueueMessage).
  • 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 PublishBatch failure 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.
  • 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=0 plus 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_status stays 1 while 100% of events fail. The only signals are an error counter with no reason (m2) and sampled logs.
  • Fix:
    • Capability check. The server returns its mode in PingResponse.message (today a constant "pong"). The node publishes only while the mode is legacy-telemetry. Otherwise it logs one ERROR, sets a gauge and re-probes every 30 s, without failing boot. As a bonus, a server-side paused mode 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 events VALIDATION_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 Internal as transient (see Rollout plan).

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), splitMessagesByRequestSize re-runs proto.Size over 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.

M8. On LLO nodes, unsupported types cause a per-message WARN flood and a drop metric that never returns to zero.

  • Where:
  • What happens:
    • Every llo-report message fails in newEvent and calls drop().
    • Send resets dropCount on every successful enqueue (L127), and llo-report interleaves 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.
    • errorCount never resets either, so error logging scales with message rate.
  • 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.
  • 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-observation flood, 10/10 ocr2-median messages were dropped.
  • Not regressions:
  • Fix: a per-type admission quota in Send (e.g. ≤ X% of BufferSize, 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 in methodConfig/retryPolicy, so gRPC ignores it. Measured locally: 1 attempt, versus 3 for a correctly wrapped control config.
  • So a 1 s Unavailable blip drops up to MaxBatchSize messages. At-most-once is fine for telemetry, but please document it.
  • The risk: once upstream fixes the JSON, a routine bump starts retrying RESOURCE_EXHAUSTED three times against an overloaded server.
  • Discard-on-stop, the split (M7), seqnum stamping 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.

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-selectors fail 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/type domains (common.go:67-107). The comment at L166 says they select the topic, but the legacy server routes on telemetrytype.
  • 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.info even with the flag off (manager.go:76-79, 196-210).
  • Exported surface changes:
    • NewManager's signature changed;
    • Manager.MonitoringEndpointGenerator is removed;
    • TelemPayload lost Domain/Entity;
    • config.TelemetryIngress gained 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.ChainID is never populated (chip_ingress.go:116-122).
  • onPublished captures the whole payload, including the Telemetry bytes, but needs only TelemType and ContractID.

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 DecodeTOML uses DisallowUnknownFields (chainlink-common pkg/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 PublishBatchTelemetry symbol or RPC in chainlink, chainlink-common, chainlink-protos or atlas. The PR calls the existing PublishBatch RPC through chainlink-common/pkg/chipingress/batch.Client.
  • Client side: done. All options used (WithEventClone, WithShutdownTimeout, WithClientName, ErrorCodeFor, WithNOPLookup) exist in the already-pinned chipingress v0.0.11-0.20260915184316-2730f1867c92. go.mod bumps 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 with ENABLE_LEGACY_TELEMETRY, and there is no new RPC.
  • Implications:
    1. 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).
    2. The real dependency is the server mode. It must be live before any node flips the flag.
    3. Older servers accept the events and route them by source/type (m8).
    4. Retries, splitting and discard-on-stop are library behavior and can change with a bump (m1).
    5. Still open: CSA-principal authorization for the legacy topics, and the server's receive limit compared with the node's request size.

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 != nil in six places (L58, 82, 111, 136, 182, 207).
  • WSRPC, the default, is inline code in newEndpoint rather 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
}
  • wsrpcTransport holds today's newEndpoint body. URL/ServerPubKey are required only there, which fixes m6.
  • chipTransport is 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, and NewApplication loses its 28-line block.
  • Optional, for rollout: a fallbackTransport that builds both transports and checks one atomic chipReady per message. chipReady is 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:

  1. Decidable from the TOML alone → reject in ValidateConfig. This covers types, ranges, host:port syntax and cross-field checks. The node refuses to boot like it would for any invalid TOML, and node validate and CI catch it. These checks run even with the feature off.
  2. 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.
  3. Construction error after validation passed → fail open. Log an ERROR and use a Noop transport. Never return nil, err from NewApplication because 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.json is 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 PublishBatch with Internal. The healthy records are still written, but the node counts them as failed.
  • Strict-mode rejections are returned as Internal with the reason lost, so the node's breaker must treat Internal as transient.
  • Also needed before the first flip: mode in Ping (M6), and strip-not-reject for server-owned extensions (M5).
  1. 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.
  2. 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_pings GOAWAY 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.
  3. 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 splitMessagesByRequestSize CPU share, WARN rate, /health 207s, and any "Shutdown grace period … exceeded" lines.
  4. 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_sent counts batches, chip counts messages.
  5. 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 MaxConcurrentSends in flight when the server is slow.
  6. 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-at p99 below an agreed bound.
  • No telemetry-caused shutdown hard exits or /health flaps.

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 BufferSize queued 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 TestScripts goldens (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 and x-include-nop-info on 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

  1. Is ChipIngressEnabled=true set anywhere today (infra, CRIB, NOP configs)? See M1.
  2. Can atlas#13712 return its mode, plus a paused state, in PingResponse.message? See M6.
  3. Are node CSA principals authorized for the legacy topics/domains on the dedicated deployment?
  4. Do you agree that telemetry construction errors should fail open instead of aborting boot?
  5. Is the fallback transport or a shadow mode in scope for this PR, or a follow-up? See M10.
  6. What are typical and maximum LLO/OCR3 payload sizes? They're needed for a byte bound (M9).
  7. Who owns the O(n) split fix in chainlink-common? Beholder's emitter benefits too.
  8. Is a static, non-expiring token (AuthHeadersTTL=0) acceptable for this endpoint?
  9. Can we get per-CSA-key message rates from OTI to size ChipIngressMaxBatchSize?

What's good

  • A non-blocking Send and 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;
    • partitionkey handles head-report's empty contract id;
    • sentat is now unix ns, fixing the old RFC3339;
    • the CSA key matches the authenticated key;
    • WithNOPLookup is 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.

Bump pkg/chipingress to b866ad79 (chainlink-common #2461) in every module,
so Close publishes queued telemetry instead of discarding it, and restate
the shutdown docs. The root chainlink-common from develop already includes
the durable emitter fix (#2465), which had to land first.
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

CORA - Pending Reviewers

Codeowners Entry Overall Num Files Owners
* ⏳ 34 @smartcontractkit/foundations, @smartcontractkit/core
/core/services/telemetry/ 💬 4 @smartcontractkit/data-tooling, @smartcontractkit/core
/core/web/resolver/ ⏳ 3 @smartcontractkit/foundations, @smartcontractkit/core
go.mod ⏳ 8 @smartcontractkit/core, @smartcontractkit/foundations
go.sum ⏳ 8 @smartcontractkit/core, @smartcontractkit/foundations
integration-tests/go.mod ⏳ 1 @smartcontractkit/core, @smartcontractkit/devex-tooling, @smartcontractkit/foundations
integration-tests/go.sum ⏳ 1 @smartcontractkit/core, @smartcontractkit/devex-tooling, @smartcontractkit/foundations
/docs/CONFIG.md ⏳ 1 @smartcontractkit/foundations, @smartcontractkit/core, @smartcontractkit/devrel

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.
@skippaDaBitFlippa

Copy link
Copy Markdown
Author

Hey @pkcll, pushed 2b9a66e with the config surface items and the small fixes:

  • M1: no default ChipIngressEndpoint anymore, it's required when the flag is on.
  • M2: validation runs with the flag off too. Bounds on all the chip knobs, host:port syntax and port range, and ChipIngressInsecureConnection is rejected for chain.link hosts.
  • M3: added ChipIngressSendInterval, ChipIngressSendTimeout and ChipIngressMaxGRPCRequestSize. The WSRPC SendInterval/SendTimeout are back to WSRPC only.
  • M4: ChipIngressDrainTimeout defaults to 1s, and with the flag on it has to be under half of ShutdownGracePeriod.
  • M5: the node no longer sends receivedat. The test pins the exact extension set.
  • M7: ChipIngressMaxBatchSize defaults to 100.
  • M8: unsupported types go to a new telemetry_client_messages_unsupported counter instead of dropped, and repeated warnings are limited to one line per type every 30s with a suppressed count.
  • m3: a failed Ping only drops the gauge. The service goes unhealthy after about a minute of failed pings and clears on the next good one.
  • m6: in chip mode endpoints only need Network and ChainID.
  • m7: flag on with no Endpoints logs a warning and skips building the chip client.
  • Nitpicks: removed TelemPayload.ChainID, and onPublished only captures the type and contract ID.

m1 and m4 were already covered by the pkg/chipingress bump in e7e0ceb. Retries are opt-in now, so nothing retries, and Close publishes what's queued within the drain timeout and counts the rest as send errors.

I also removed the changeset. For m9, the API changes are NewManager's signature, the removed MonitoringEndpointGenerator, TelemPayload dropping Domain/Entity/ChainID, and new config.TelemetryIngress methods.

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 receivedat gone from the node, my local chip-ingress run didn't stamp ce_receivedat or received-at on the records, so the server needs to set it if consumers still read it.

@github-actions

Copy link
Copy Markdown
Contributor

I see you updated files related to core. Please run make gocs in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@cl-sonarqube-production

Copy link
Copy Markdown

# 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

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.

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.

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.

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()

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.

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

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.

perhaps @pkcll could weigh in here as well

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, we should use OTel in addition to prom metrics

Comment on lines +193 to +196
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

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.

these are technically fine to include, but most of these are going to largely be ignored on the ingress side:

  • networkname isn't useful for us, we use different names internally (chainselector is the source of truth here)
  • csapublickey fine 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 authoritative
  • partitionkey we'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))

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.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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) {

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.

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

@pkcll pkcll Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines +54 to +55
// 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.

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.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We start with 1 batch client 1 buffer 1 connection, observe and decide if we need to optimize

@pkcll pkcll left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

  • e7e0cebf merges develop and bumps pkg/chipingress to b866ad79, which adds drain-on-Stop and makes retries opt-in.
  • 2b9a66e8 addresses 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.toml
  • core/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.md was removed in 2b9a66e8. The changeset bot asked for one again on 10-15.
  • Why it matters: the PR adds nine public TOML keys under [TelemetryIngress], which DecodeTOML makes hard to remove later. It also changes exported API: the NewManager signature, the removed MonitoringEndpointGenerator, TelemPayload fields, and new config.TelemetryIngress methods.
  • 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, from PublishResponse.Results (library batch/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 sent and 3 send_errors.
    • An OK response with an empty Results counts every event as a send error (RESULTS_MISMATCH). That is fine against atlas, which always fills Results.
  • The risk: the library default could change in a bump. In strict mode the server returns Internal for 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 to TestChipIngressBatchClient_CountsPerMessage: an OK response where one result has an Error should give sent = n-1 and send_errors = 1.

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.AuthHeadersTTL in (0, 10m). It still passes validation, and then NewHeaderProvider fails 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 = 10 batches in flight. When server latency exceeds ChipIngressSendInterval (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, default ChipIngressMaxConcurrentSends to 1–2 and size it from Q9's per-node rates.

Nitpicks

  • The chain.link insecure guard is case-sensitive (types.go:1123). LEGACY.CHAIN.LINK:443 or a trailing-dot host gets past it. Normalize first with h := strings.ToLower(strings.TrimSuffix(host, ".")).
  • Endpoint syntax gaps (types.go:1113):
    • Only leading and trailing whitespace is caught, so ho st:443 passes.
    • host:443/path is rejected with "port must be between 1 and 65535", which is misleading.
    • Fix: reject strings.ContainsAny(endpoint, " \t/") with the syntax message.
  • The test helper doesn't use the shipped defaults (helpers_test.go:32-44). NewTestChipIngressBatchClient uses MaxBatchSize: 1_000, which validation now rejects, and DrainTimeout: 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-report message allocates "unsupported:"+telemType and takes the client-wide logLimiter mutex just to maybe emit a Debug line. Logging once per type (a sync.Map of 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, csapublickey and partitionkey are ignored in legacy mode; atlas docs/api.md says the node "may stop sending both". Nothing reads networkname. 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 in TestChipIngressBatchClient_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. Keep WithNOPLookup() 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 set ChipIngressEnabled=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 NoopAgent at wiring time (chip_ingress.go:56-59).
    • The Send-time check only fires for multitype endpoints, where the type arrives with each call; LLO's SendTypedLog(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.
  • chip_ingress_batch_client.go:150, Prometheus vs OTel.

    • The library already emits OTel chip_ingress.batch.* (requests, sizes, latency, splits, result mismatches) with client_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 attributes telemetry_type (about 20 values) and error_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.
  • chip_ingress_batch_client.go:147, differences from today's WSRPC path:

    Aspect WSRPC (today) Chip batch client
    Queueing One channel per (contract, type), BufferSize 100 each One shared channel, 10 000 (M9)
    Backpressure Drop newest, counted dropped Drop newest, counted dropped; unsupported types counted separately
    Retries 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 as send_errors
    Counting sent and send_errors count batches Count 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

  1. m8: is it tracked in a follow-up, or explicitly dropped now that atlas routes on telemetrytype?
  2. R3-4: does any consumer of the legacy topics depend on per-key order?
  3. 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?
  4. 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/ClearHealthCond correctly, and the test drives both directions.
  • logLimiter replaces 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.

@pkcll

pkcll commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to round 3: send with batch.WithNoAck() (chainlink-common#2491)

@skippaDaBitFlippa, one more item for the flip-blocker list. It also answers @4of9's fire-and-forget point at chip_ingress_batch_client.go:147.

What. Pass batch.WithNoAck() in NewChipIngressBatchClient. It becomes a one-line change once two chainlink-common PRs merge and pkg/chipingress is bumped: smartcontractkit/chainlink-common#2478 (the PublishBatchNoAck RPC) and smartcontractkit/chainlink-common#2491 (the batch option).

batch.WithTransactionEnabled(false), // partial delivery: per-event results (R3-2)
batch.WithNoAck(),                   // PublishBatchNoAck: server produces with acks=0; legacy telemetry is loss-tolerant

Why

  • Parity. OTI's traffic through chip-ingress is already produced with acks=0, through the hard-coded oti-telemetry-shared principal branch in chip-ingress write_service.go. With plain PublishBatch, the node would wait for broker acks, which today's legacy traffic doesn't.
  • Fire-and-forget, explicitly. Retries are already off. With NoAck, the server also answers without waiting for broker acknowledgement, so a slow broker no longer holds the batch client's send slots.
  • It shrinks three risks from earlier rounds:
    • Shorter in-flight time, so the 1s DrainTimeout covers more of the queue at shutdown (M4).
    • Shorter backlogs after a slow period, so fewer full batches hit the O(n²) split (M7).
    • A smaller reordering window across concurrent sends (R3-4).
  • Nothing else changes. Validation, partial delivery and positional per-event results are the same as for PublishBatch (EthTxManager has more resilient retry behavior and nonce tracking (WIP) #2491). The per-message counters and callbacks therefore keep working unchanged, including missing_nop_info and legacy_validation_error.

What changes on the node

  • telemetry_client_messages_sent then means "validated and handed to the producer", not "persisted". Please say so in the metric help text and in the onPublished comment.
  • No new TOML key. Legacy telemetry is always loss-tolerant, and the dedicated legacy deployment gets the RPC before any node flips, so hard-code the option.
  • Tests: startBatchClient mocks PublishBatch. Switch it to PublishBatchNoAck, and assert that PublishBatch is never called.

Rollout prerequisites (please add these to the flip gate)

  1. chainlink-common#2478 and EthTxManager has more resilient retry behavior and nonce tracking (WIP) #2491 are merged, and pkg/chipingress is bumped in every module.
  2. smartcontractkit/atlas#13777 (server PublishBatchNoAck) is deployed on the dedicated legacy deployment before the first flip. There is no client fallback: a server without it fails every batch with Unimplemented, while Ping stays green. The M6 breaker should treat Unimplemented as permanent.
  3. atlas#13777 and smartcontractkit/atlas#13712 are reconciled. fix/remove cw ctx #13777 branches from master, and on the NoAck path a local producer error (for example UNKNOWN_TOPIC_OR_PARTITION, or a full buffer) returns Internal for the whole request. That undoes BCI-3619 Fix TestHeadTracker_CallsHeadTrackableCallbacks flaky test #13712's per-event produce failures (405ca32f06) for exactly this traffic: one missing topic would again count every healthy event in the batch as a send error. In partial-delivery mode, ProduceNoAck should return per-record errors, as Produce does after BCI-3619 Fix TestHeadTracker_CallsHeadTrackableCallbacks flaky test #13712. cc @thomaska
  4. system-tests/tests/test-helpers/chip-testsink embeds UnimplementedChipIngressServer. It still compiles after the bump but answers Unimplemented for NoAck. No system test enables legacy chip mode today, so this only matters for a future e2e test of this path.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-publish Build and Publish image to SDLC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants