[1/2] chipingress: expose drop failure classification for partial delivery metrics#2210
Closed
pkcll wants to merge 1 commit into
Closed
[1/2] chipingress: expose drop failure classification for partial delivery metrics#2210pkcll wants to merge 1 commit into
pkcll wants to merge 1 commit into
Conversation
4 tasks
Contributor
📊 API Diff Results
|
pkcll
force-pushed
the
chipingress-partial-delivery-cl-nodes
branch
4 times, most recently
from
July 3, 2026 20:41
59011ac to
d16297c
Compare
Contributor
✅ API Diff Results -
|
pkcll
force-pushed
the
chipingress-partial-delivery-cl-nodes
branch
7 times, most recently
from
July 21, 2026 05:57
f74f0a3 to
6d7cc16
Compare
pkcll
changed the title
pkcll
force-pushed
the
chipingress-partial-delivery-cl-nodes
branch
from
July 21, 2026 06:07
6d7cc16 to
5b8513f
Compare
pkcll
changed the title
pkcll
marked this pull request as ready for review
July 21, 2026 06:14
product-security-plaid-production
Bot
requested review from
4of9,
engnke,
fouadkada,
kirqz23 and
thomaska
July 21, 2026 06:14
Contributor
There was a problem hiding this comment.
Pull request overview
Adds shared error classification utilities to pkg/chipingress/batch so downstream components (e.g., beholder in the follow-up PR) can emit consistent partial-delivery / send/queue failure metrics and logs.
Changes:
- Introduces
DropFailureandClassifyDropFailure(err)to map batch send/queue errors into{error_type, error_code, error_reason}. - Adds unit tests covering common gRPC statuses, partial-delivery
PublishError, and local queue conditions. - Exports
ErrMessageBufferFull/ErrClientShutdownsentinels and updatesQueueMessageto return them for reliableerrors.Ismatching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/chipingress/batch/drop_failure.go | Adds the DropFailure model + ClassifyDropFailure helper used for metric/log dimensions. |
| pkg/chipingress/batch/drop_failure_test.go | Adds coverage for error classification across partial delivery, gRPC, and local queue errors. |
| pkg/chipingress/batch/client.go | Exports sentinel errors and returns them from QueueMessage to enable stable classification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+74
to
+78
| return DropFailure{ | ||
| ErrorType: ErrorTypeRPCError, | ||
| ErrorCode: codes.Unknown.String(), | ||
| ErrorReason: err.Error(), | ||
| } |
Comment on lines
+74
to
+80
| { | ||
| name: "unknown error", | ||
| err: errors.New("something else"), | ||
| wantType: batch.ErrorTypeRPCError, | ||
| wantCode: codes.Unknown.String(), | ||
| wantReason: "something else", | ||
| }, |
| ErrorType string // partial_delivery | rpc_error | buffer_full | client_error | ||
| ErrorCode string // PUBLISH_ERROR_CODE_* or gRPC code name (DeadlineExceeded, …) | ||
| ErrorReason string // server reason, status message, or client error text | ||
| } |
Contributor
There was a problem hiding this comment.
It seems that you don't really need a separate type DropFailure here.
In #2266, you can:
- use
sendErr.Error()instead ofdrop.ErrorReason, - use only ErrorCode instead of ErrorType (one can infer one from another)
- In this PR (or you can drop it and update beholder: handle partial-delivery errors via metric and log suppression #2266 directly)have one method generating
ErrorCodewith separate if's based onerrors.AsType.
pavel-raykov
requested changes
Jul 21, 2026
pkcll
force-pushed
the
chipingress-partial-delivery-cl-nodes
branch
from
July 21, 2026 21:27
65f2ba8 to
e8d7286
Compare
Comment on lines
+36
to
+41
| if errors.Is(err, ErrMessageBufferFull) { | ||
| return ErrMessageBufferFull.Error() | ||
| } | ||
| if errors.Is(err, ErrClientShutdown) { | ||
| return ErrClientShutdown.Error() | ||
| } |
Comment on lines
+18
to
+22
| // ErrorCodeFor returns a bounded error code string for a batch send/queue error. | ||
| // The returned code is intended for metric dimensions; callers can infer the | ||
| // error category from the code: | ||
| // - PUBLISH_ERROR_CODE_* -> partial_delivery | ||
| // - "buffer_full" -> buffer_full |
Comment on lines
+50
to
+54
| { | ||
| name: "buffer full", | ||
| err: batch.ErrMessageBufferFull, | ||
| want: batch.ErrMessageBufferFull.Error(), | ||
| }, |
Comment on lines
+55
to
+59
| { | ||
| name: "client shutdown", | ||
| err: batch.ErrClientShutdown, | ||
| want: batch.ErrClientShutdown.Error(), | ||
| }, |
Comment on lines
+22
to
+25
| var ( | ||
| ErrMessageBufferFull = errors.New("message buffer is full") | ||
| ErrClientShutdown = errors.New("client is shutdown") | ||
| ) |
Comment on lines
+67
to
+72
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| assert.Equal(t, tt.want, batch.ErrorCodeFor(tt.err)) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
pkg/chipingress/batch/drop_failure.go:40
ErrorCodeForcurrently returns the full sentinel error messages for buffer-full and shutdown ("message buffer is full" / "client is shutdown"), but the function comment describes stable metric codes ("buffer_full" / "client_shutdown"). Returning the human-readable strings couples metric dimensions to error wording and doesn't match the documented values.
if errors.Is(err, ErrMessageBufferFull) {
return ErrMessageBufferFull.Error()
}
if errors.Is(err, ErrClientShutdown) {
return ErrClientShutdown.Error()
pkg/chipingress/batch/drop_failure_test.go:54
- Test expects
ErrorCodeForto return the sentinel error string for buffer-full, but the helper's doc (and intended metric code) is"buffer_full". Align the test expectation with the stable code value.
name: "buffer full",
err: batch.ErrMessageBufferFull,
want: batch.ErrMessageBufferFull.Error(),
},
pkg/chipingress/batch/drop_failure_test.go:59
- Test expects
ErrorCodeForto return the sentinel error string for shutdown, but the helper's doc (and intended metric code) is"client_shutdown". Align the test expectation with the stable code value.
name: "client shutdown",
err: batch.ErrClientShutdown,
want: batch.ErrClientShutdown.Error(),
},
pkg/chipingress/batch/drop_failure.go:53
- PR description mentions adding
batch.ClassifyDropFailureto provide shared metric dimensions (error_type,error_code,error_reason), but this file currently only exposesErrorCodeFor. Consider adding the promised helper so downstream callers (e.g. beholder) can consistently derive all dimensions without re-implementing logic.
return ErrorTypeClientError
}
Comment on lines
+75
to
+78
| func TestErrorCodeFor_nil(t *testing.T) { | ||
| t.Parallel() | ||
| require.Empty(t, batch.ErrorCodeFor(nil)) | ||
| } |
pkcll
force-pushed
the
chipingress-partial-delivery-cl-nodes
branch
2 times, most recently
from
July 21, 2026 21:41
57bb513 to
8cd89c4
Compare
Contributor
Author
|
Folding this into #2266 — moved drop_failure.go there and simplified ErrorCodeFor to return (errorType, errorCode) directly, so there's no cross-PR dependency to coordinate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 of 2 — adds the chipingress batch client helpers needed for the beholder partial-delivery metric change.
ErrMessageBufferFullandErrClientShutdownsentinels frompkg/chipingress/batch.batch.ClassifyDropFailureto map send/queue errors into shared metric dimensions (error_type,error_code,error_reason).Part 2: #2266
Jira: INFOPLAT-13901