Allow customizing spam filtering in event client library #103918
Conversation
k8s-ci-robot
added
release-note
|
Hi @olagacek. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
sig/api-machinery
| duplicateEvent := makeEvent("duplicate", "me again", makeObjectReference("Pod", "my-pod", "my-ns")) | ||
| similarEvent := makeEvent("first", "me again", makeObjectReference("Pod", "my-pod", "my-ns")) | ||
| testCases := map[string]struct { | ||
| previousEvents []v1.Event |
There was a problem hiding this comment.
Nit, looks like previous event is always the same, it might be more readable to not include previousEvents field in the test case
There was a problem hiding this comment.
Removed previousEvents field
| newEvent v1.Event | ||
| expectedEvent v1.Event | ||
| intervalSeconds int | ||
| burstSize int |
There was a problem hiding this comment.
ditto for burstSize and expected Skip. Not need to include in test case if they don't change between tests.
There was a problem hiding this comment.
burstSize is not included in test case anymore, but expectedSkip actually changes between the test cases :)
| "") | ||
| } | ||
| firstEvent := makeEvent("first", "i am first", makeObjectReference("Pod", "my-pod", "my-ns")) | ||
| duplicateEvent := makeEvent("duplicate", "me again", makeObjectReference("Pod", "my-pod", "my-ns")) |
There was a problem hiding this comment.
For me those names are reversed. Duplicate event should be exactly the same as the original and similar can differ. Can we make it more clearer in names or in test arguments what are differences?
There was a problem hiding this comment.
Renamed the variables, hope they are more descriptive now
|
/assign @serathius |
k8s-ci-robot
added
triage/accepted
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olagacek, serathius 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 |
k8s-ci-robot
added
the
approved
kubernetes#47367 introduced client side event spam filtering to reduce the API server/etcd pressure of processing repeated events. However, it use only Source and InvolvedObject as key, so it can also drop important and unique events. Slowing down the diagnosing significantly. kubernetes#103918 try to resolve this by adding a new SpamKeyFunc to customize the key used. But this is still not ideal, because every user needs to dig into the code of client-go to figure out why his event does not appear. If the EventBroadcaster is initialized in a library, it may be even harder to set the SpamKeyFunc. I propose to only drop PATCH events, which is the intent of the original proposal. This patch deprecates SpamKeyFunc, and effectively fixed it to the return value of EventAggregate(). For the users who don't set SpamKeyFunc, they should get exactly more events then before, so this is not breaking for them. Other users are likely just adding Reason to the key. We already do this now.
PR kubernetes#47367 introduced client side event spam filtering to reduce the API server/etcd pressure of processing repeated events. However, it use only Source and InvolvedObject as key, so it can also drop important and unique events. Slowing down the diagnosing significantly. PR kubernetes#103918 try to resolve this by adding a new SpamKeyFunc to customize the key used. But this is still not ideal, because every user needs to dig into the code of client-go to figure out why his event does not appear. If the EventBroadcaster is initialized in a library, it may be even harder to set the SpamKeyFunc. I propose to only drop PATCH events by default, which is the intent of the original proposal. For the users who don't set SpamKeyFunc, they should get exactly more events.
PR kubernetes#47367 introduced client side event spam filtering to reduce the API server/etcd pressure of processing repeated events. However, it use only Source and InvolvedObject as key, so it can also drop important and unique events. Slowing down the diagnosing significantly. PR kubernetes#103918 try to resolve this by adding a new SpamKeyFunc to customize the key used. But this is still not ideal, because every user needs to dig into the code of client-go to figure out why his event does not appear. If the EventBroadcaster is initialized in a library, it may be even harder to set the SpamKeyFunc. I propose to only drop PATCH events by default, which is the intent of the original proposal. For the users who don't set SpamKeyFunc, they should get exactly more events.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Current implementation
Event client library has aggregation and spam filtering implemented. While aggregation can be fully customized, spam filtering functionality doesn't allow overriding spamKey func - function that is responsible for creating a key (based on an event) which is used to detect "spam" events. Currently used
getSpamKey()function takes into accountSourceandInvolvedObjectof an event. Per each such keyBurstSizeevents can be sent.Changes in PR
Allow customizing spamKey func in
CorrelatorOptionsofEventCorrelator.Why allow customizing spamKey func?
For some use cases it can be useful to also include for example
Reasonfield of an event in spamKey. For example, if we havennormal events for a specific Object with the same reason andn+1-th event is e.g warning with a different reason we would like to have it emitted, while filtering n-1 normal events.This PR does not change default behavior and doesn't introduce any breaking change. Current users of client library won't experience any change, unless they explicitly modify
CorrelatorOptions.Which issue(s) this PR fixes:
None
Does this PR introduce a user-facing change?
/cc @serathius @dashpole @logicalhan
/sig instrumentation