Drop non-event messages #1212
Drop non-event messages #1212
Conversation
|
Need to wait for #1208 to be merged so the unit test failure will fix itself |
pkg/broker/handler/handler.go
Outdated
| // The following errors can be returned by ToEvent and are not retryable. | ||
| // TODO Should ToEvent consolidate them and return the genric ErrCannotConvertToEvent? | ||
| if errors.Is(err, binding.ErrCannotConvertToEvent) || errors.Is(err, binding.ErrNotStructured) || errors.Is(err, binding.ErrUnknownEncoding) || errors.Is(err, binding.ErrNotBinary) { | ||
| logging.FromContext(ctx).Error("failed to convert received message to an event, check the msg format", zap.Any("message", msg), zap.Error(err)) |
There was a problem hiding this comment.
Max message data size is 10MB. Worst case, if all messages in the topic are the wrong format and very large we use a lot of resources logging them. Maybe print the message id and first N bytes of data?
There was a problem hiding this comment.
Maybe log both error and debug. Error has the shorter message and debug has the full message?
There was a problem hiding this comment.
I was debating whether to log the full msg. This should really not happen in normal conditions. Since we don't have a DLQ, without logging it's difficult for the user to debug.
Maybe picking a big N such as 5000 is good enough to cover most cases?
There was a problem hiding this comment.
Log full msg in debug, and truncate the Data field to 2000 chars in error (other fields are either fixed size or a map of attributes)
pkg/broker/handler/handler.go
Outdated
| // The following errors can be returned by ToEvent and are not retryable. | ||
| // TODO Should ToEvent consolidate them and return the genric ErrCannotConvertToEvent? | ||
| if errors.Is(err, binding.ErrCannotConvertToEvent) || errors.Is(err, binding.ErrNotStructured) || errors.Is(err, binding.ErrUnknownEncoding) || errors.Is(err, binding.ErrNotBinary) { | ||
| logging.FromContext(ctx).Error("failed to convert received message to an event, check the msg format", zap.Any("message", msg), zap.Error(err)) |
There was a problem hiding this comment.
Maybe log both error and debug. Error has the shorter message and debug has the full message?
|
/unhold as #1208 is merged |
|
/hold cancel |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grantr, liu-cong The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The following is the coverage report on the affected files.
|
|
/retest |
|
/unhold |
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-wi-tests: |
|
/retest |
Fixes #1178
Proposed Changes
Release Note
Docs