Skip to content

Conversation

@Evedel
Copy link

@Evedel Evedel commented Nov 19, 2022

This PR is bundled with a number of changes all regarding how event age is handled. Please let me know if you want to slice them differently or what your thoughts are in general.

  1. Dropping event handling OnUpdate (diff link)

  2. Adding warning and prometheus metrics for when an event is discarded (diff link)

    • Currently on main, if the eventAge > throttlePeriod => it is going to be silently dropped.
      Not only is it making debugging and choosing correct config options particularly hard (blind guessing), but also it is impossible to set promethues alarms for if a cluster has outgrown previously correct configs.
      Test case:
      • run two deployment of kubernetes-event-exporter
        • one is the exporter (kubeQPS:100, kubeBurst:500, throttledPeriod:5,logLevel: Info)
      • watch for only new events with k get events -A --watch-only > only-new-events.log
      • scale second deployments k scale deployment event-exporter-test --replicas 200
      • no signs of client-side throttling
      • no errors or warnings in logs
      • but the exporter only processed a small portion of events
    • With this PR and the same settings:
      • logs are clearly showing a problem and how to solve it
      • increase( events_discarded [1m]) can be used for alarms
        image
  3. Renaming config.throttlePeriod to config.maxEventAgeSeconds (diff link)

    • this is extremely confusing, as this parameter has nothing to do with the request throttling as shown in tests in (2)
    • config validation is added to accept both parameter names (for backward compatibility)
    • all examples/code/docs updated to use the new name

@Evedel Evedel marked this pull request as ready for review November 19, 2022 02:14
@Evedel Evedel force-pushed the add-more-visibility-for-dropped-events branch 2 times, most recently from 4b3e10b to b50e44a Compare November 21, 2022 01:38
@Evedel Evedel force-pushed the add-more-visibility-for-dropped-events branch from b50e44a to 4412484 Compare November 21, 2022 01:39
@Evedel
Copy link
Author

Evedel commented Nov 24, 2022

Hello @mustafaakin, any chance you will be able to have a look?

@mustafaakin
Copy link

Sorry for late response. Your contribution is welcome, and thank you, we should handle it better. Can you fix the recent conflicts and send it again?

@Evedel
Copy link
Author

Evedel commented Dec 19, 2022

Not a problem @mustafaakin. Ready for review now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants