beholder: handle partial-delivery errors via metric and log suppression by pkcll · Pull Request #2266 · smartcontractkit/chainlink-common

pkcll · 2026-07-21T05:58:35Z

With partial delivery enabled, per-event PublishError failures are now returned in the callback. This PR:

Adds batch.ErrorCodeFor to pkg/chipingress/batch/client.go, classifying a send/queue error into a bounded error_code string via errors.As / errors.Is / status.FromError.
Suppresses ERROR logging for per-event PublishError failures — detected directly via errors.As(sendErr, &pubErr) in beholder — since they're already captured by the chip_ingress.events_dropped metric.
Continues to log other send/queue errors at ERROR, enriched with error_code and error_reason (sendErr.Error()).
Adds an error_code attribute to the chip_ingress.events_dropped metric.

Self-contained: the chipingress-side classification helper (previously split out as its own PR, #2210, closed) now lives in this PR alongside its beholder consumer, so there's no cross-PR go.mod dependency to coordinate.

Jira: INFOPLAT-13901

Copilot

Pull request overview

This PR updates the Beholder chip-ingress batch emitter to classify drop failures into consistent metric/log dimensions, reducing log noise for partial delivery while preserving chip_ingress.events_dropped as the primary operational signal.

Changes:

Use batch.ClassifyDropFailure to populate chip_ingress.events_dropped attributes (error_type, optional error_code) and enrich error logs for non-partial failures.
Suppress error-level logging for partial-delivery drops (metric-only for that class of failure).
Add/adjust tests to validate metric dimensions for partial delivery and RPC failures; bump root dependency on pkg/chipingress.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File	Description
pkg/beholder/batch_emitter_service.go	Classifies drop failures and adjusts metric/log behavior (no error log for partial delivery).
pkg/beholder/batch_emitter_service_test.go	Adds test coverage for partial-delivery classification and RPC error classification in metrics/log fields.
go.mod	Bumps `github.com/smartcontractkit/chainlink-common/pkg/chipingress` dependency version.
go.sum	Updates checksums for the bumped `pkg/chipingress` dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-07-21T06:18:11Z

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-common

View full report

Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

+// dropMetricAttrsFor returns a measurement option for the eventsDropped counter.
+// Not cached — drop paths are not on the hot path.
+//
+// error_reason is deliberately excluded: it is free-form text from the server/gRPC
+// stack (e.g. validation messages, status details) and is not a bounded value, so using
+// it as a metric attribute would create unbounded cardinality. domain/entity/error_type/


Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

+				errorCode := batch.ErrorCodeFor(sendErr)
+				errorType := errorTypeFromCode(errorCode)
+				e.metrics.eventsDropped.Add(ctx, 1, e.dropMetricAttrsFor(domain, entity, errorType, errorCode))


+	switch {
+	case code == "":
+		return ""
+	case strings.HasPrefix(code, chipingress.PublishErrorCode(0).String()) || strings.HasPrefix(code, "PUBLISH_ERROR_CODE_"):
+		return batch.ErrorTypePartialDelivery
+	case code == batch.ErrMessageBufferFull.Error():
+		return batch.ErrorTypeBufferFull
+	case code == batch.ErrClientShutdown.Error() || code == batch.ErrorTypeClientError:
+		return batch.ErrorTypeClientError
+	default:
+		return batch.ErrorTypeRPCError
+	}


thomaska

👍 💯 Added a small comment, but overall sgtm!

thomaska · 2026-07-22T12:24:36Z

+
+	var pubErr *PublishError
+	if errors.As(err, &pubErr) {
+		return pubErr.Code.String()


This can be problematic for ErrCodeResultsMismatch that is chipingress.PublishErrorCode(-1) ie. not in the 0-4 range of the proto enum, so this will return a cryptic "-1"
Maybe we could do explicit handling here:

var pubErr *PublishError if errors.As(err, &pubErr) { if pubErr.Code == ErrCodeResultsMismatch { return "results_mismatch" } return pubErr.Code.String() }

…elivery-cl-nodes-beholder Resolves go.mod/go.sum and pkg/beholder/batch_emitter_service_test.go conflicts. Addresses PR review comment: ErrorCodeFor returns 'results_mismatch' for the synthetic ErrCodeResultsMismatch (-1) instead of the cryptic proto String() value.

github-actions · 2026-07-22T14:46:17Z

✅ API Diff Results - `github.com/smartcontractkit/chainlink-common/pkg/chipingress`

✅ Compatible Changes (3)

`batch` (3)

ErrClientShutdown — ➕ Added
ErrMessageBufferFull — ➕ Added
ErrorCodeFor — ➕ Added

📄 View full apidiff report

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

pkg/chipingress/batch/client.go:81

ErrorCodeFor currently uses ErrMessageBufferFull.Error() / ErrClientShutdown.Error() as the returned error_code. That couples metric attribute values to human-readable error messages (including spaces), so a future wording change would silently change the metric dimension values. Prefer returning stable, explicitly-named codes (e.g. "message_buffer_full" / "client_shutdown") and keeping the human-readable text only in logs.

	if errors.Is(err, ErrMessageBufferFull) {
		return ErrMessageBufferFull.Error()
	}
	if errors.Is(err, ErrClientShutdown) {
		return ErrClientShutdown.Error()
	}

+				// Partial delivery is not logged: it is a per-event, often persistent
+				// server-side condition (e.g. missing schema) that would otherwise log on
+				// every dropped event. chip_ingress.events_dropped (error_code) already
+				// captures that it's happening and roughly why; the full reason isn't
+				// needed at fleet-wide log volume.
+				var pubErr *batch.PublishError
+				if !errors.As(sendErr, &pubErr) {
+					e.eng.Errorw("failed to emit to chip ingress",
+						"error", sendErr,
+						"error_code", errorCode,
+						"error_reason", sendErr.Error(),
+						"domain", domain,
+						"entity", entity,
+					)
+				}


Copilot review feedback: preserve the client_name attribute on chip_ingress.events_dropped when adding error_code, so existing dashboards/alerts keep working.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

pkg/chipingress/batch/client.go:27

These exported sentinel errors don’t have doc comments. If the repo’s linters enforce comments on exported identifiers (common in Go), this will fail lint and also makes it less clear to consumers that these are intended for errors.Is matching from QueueMessage / batching operations.

var (
	ErrMessageBufferFull = errors.New("message buffer is full")
	ErrClientShutdown    = errors.New("client is shutdown")
)

pkg/beholder/batch_emitter_service.go:242

dropMetricAttrsFor intentionally isn’t cached, but it currently builds an attribute.Set (attribute.NewSet + WithAttributeSet) on every drop. Creating a Set typically does extra work (dedup/sort) and allocations; since partial-delivery drops can be high-volume, prefer WithAttributes here to avoid the Set construction cost.

	if errorCode != "" {
		attrs = append(attrs, attribute.String("error_code", errorCode))
	}
	return otelmetric.WithAttributeSet(attribute.NewSet(attrs...))
}

Updates the root go.mod/go.sum to use the merged main commit of the chipingress submodule.\nThis resolves the dependency validation failure that required chipingress pseudo-versions to exist on main.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pkg/beholder/batch_emitter_service.go:176

This log-suppression condition matches any *batch.PublishError. However, the batch client also uses PublishError for the synthetic ErrCodeResultsMismatch case (pkg/chipingress/batch/client.go:405-409), which is a client/server contract issue rather than a persistent per-event validation failure. As written, results-mismatch errors will be silently dropped from logs too; consider still logging those (perhaps at ERROR) while continuing to suppress normal per-event PublishErrors.

				// Partial delivery is not logged: it is a per-event, often persistent
				// server-side condition (e.g. missing schema) that would otherwise log on
				// every dropped event. chip_ingress.events_dropped (error_code) already
				// captures that it's happening and roughly why; the full reason isn't
				// needed at fleet-wide log volume.
				var pubErr *batch.PublishError
				if !errors.As(sendErr, &pubErr) {
					e.eng.Errorw("failed to emit to chip ingress",

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.want, ErrorCodeFor(tt.err))
+		})
+	}


Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

pkg/beholder/batch_emitter_service.go:183

The log-suppression condition currently suppresses all batch.PublishError instances. This also includes the synthetic ErrCodeResultsMismatch case (server returned an unexpected number of results), which is a client/server-contract issue and is likely worth logging (it’s not the high-volume, persistent per-event validation case the suppression is intended for). Consider only suppressing logs for “real” per-event publish errors, while still logging results-mismatch (and any other non-persistent) failures.

				var pubErr *batch.PublishError
				if !errors.As(sendErr, &pubErr) {
					e.eng.Errorw("failed to emit to chip ingress",
						"error", sendErr,
						"error_code", errorCode,
						"error_reason", sendErr.Error(),
						"domain", domain,
						"entity", entity,
					)
				}

pkcll force-pushed the chipingress-partial-delivery-cl-nodes-beholder branch 3 times, most recently from 83e688e to 204caa8 Compare July 21, 2026 06:00

pkcll mentioned this pull request Jul 21, 2026

[1/2] chipingress: expose drop failure classification for partial delivery metrics #2210

Closed

pkcll force-pushed the chipingress-partial-delivery-cl-nodes-beholder branch from 204caa8 to 8aa3f31 Compare July 21, 2026 06:08

Copilot AI review requested due to automatic review settings July 21, 2026 06:08

Copilot started reviewing on behalf of pkcll July 21, 2026 06:08 View session

pkcll changed the title ~~beholder: remove partial delivery WARN log, keep metric only~~ [2/2] beholder: remove partial delivery WARN log, keep metric only Jul 21, 2026

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Comment thread pkg/beholder/batch_emitter_service_test.go

pkcll changed the title ~~[2/2] beholder: remove partial delivery WARN log, keep metric only~~ [2/2] beholder: handle partial-delivery errors via metric and log classification Jul 21, 2026

beholder: remove partial delivery WARN log, keep metric only

74cdc15

Copilot AI review requested due to automatic review settings July 21, 2026 06:16

pkcll force-pushed the chipingress-partial-delivery-cl-nodes-beholder branch from 8aa3f31 to 74cdc15 Compare July 21, 2026 06:16

Copilot started reviewing on behalf of pkcll July 21, 2026 06:17 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

pkcll marked this pull request as ready for review July 21, 2026 06:23

pkcll requested review from a team as code owners July 21, 2026 06:23

product-security-plaid-production Bot requested review from 4of9, bolekk, engnke, fouadkada, kirqz23 and thomaska July 21, 2026 06:23

beholder: use ErrorCodeFor instead of ClassifyDropFailure

0a5c31d

Copilot AI review requested due to automatic review settings July 21, 2026 21:28

Copilot started reviewing on behalf of pkcll July 21, 2026 21:28 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

chipingress: drop DropFailure, add ErrorCodeFor

104555f

thomaska previously approved these changes Jul 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 22, 2026 14:45

pkcll dismissed stale reviews from thomaska and pavel-raykov via bd5fc2d July 22, 2026 14:45

Copilot started reviewing on behalf of pkcll July 22, 2026 14:45 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

beholder: add client_name to events_dropped metric

7f99ebd

Copilot review feedback: preserve the client_name attribute on chip_ingress.events_dropped when adding error_code, so existing dashboards/alerts keep working.

Copilot AI review requested due to automatic review settings July 22, 2026 15:15

Copilot started reviewing on behalf of pkcll July 22, 2026 15:15 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

pkcll mentioned this pull request Jul 22, 2026

[1/2] chipingress: add ErrorCodeFor for partial delivery error classification #2274

Merged

thomaska previously approved these changes Jul 22, 2026

View reviewed changes

bump pkg/chipingress to main pseudo-version from #2274

a09f287

Updates the root go.mod/go.sum to use the merged main commit of the chipingress submodule.\nThis resolves the dependency validation failure that required chipingress pseudo-versions to exist on main.

Copilot AI review requested due to automatic review settings July 24, 2026 14:43

pkcll dismissed thomaska’s stale review via a09f287 July 24, 2026 14:43

Copilot started reviewing on behalf of pkcll July 24, 2026 14:44 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread pkg/chipingress/batch/client_test.go

Comment on lines +2407 to +2412

for _, tt := range tests {

t.Run(tt.name, func(t *testing.T) {

t.Parallel()

assert.Equal(t, tt.want, ErrorCodeFor(tt.err))

})

}

thomaska approved these changes Jul 24, 2026

View reviewed changes

Merge branch 'main' into chipingress-partial-delivery-cl-nodes-beholder

548c6bd

Copilot AI review requested due to automatic review settings July 24, 2026 15:18

Copilot started reviewing on behalf of pkcll July 24, 2026 15:19 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

jmank88 approved these changes Jul 24, 2026

View reviewed changes

jmank88 enabled auto-merge July 24, 2026 15:26

jmank88 added this pull request to the merge queue Jul 24, 2026

Merged via the queue into main with commit 8dc00b1 Jul 24, 2026
27 of 31 checks passed

jmank88 deleted the chipingress-partial-delivery-cl-nodes-beholder branch July 24, 2026 15:38

Uh oh!

Conversation

pkcll commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 API Diff Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

thomaska left a comment

Choose a reason for hiding this comment

Uh oh!

thomaska Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ API Diff Results - github.com/smartcontractkit/chainlink-common/pkg/chipingress

✅ Compatible Changes (3)

batch (3)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pkcll commented Jul 21, 2026 •

edited

Loading

github-actions Bot commented Jul 21, 2026 •

edited

Loading

github-actions Bot commented Jul 22, 2026 •

edited

Loading

✅ API Diff Results - `github.com/smartcontractkit/chainlink-common/pkg/chipingress`

`batch` (3)