-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metrics #24
base: master
Are you sure you want to change the base?
Metrics #24
Conversation
82f9ff6
to
5b450c0
Compare
5b450c0
to
447825b
Compare
@@ -1 +1,3 @@ | |||
*.gem | |||
nclude-dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the lock file is not included in the plugins(since it depends on build env). Thats why i didnt include. I checked with other plugins too. Its the same
lib/fluent/plugin/filter_throttle.rb
Outdated
desc <<~DESC | ||
Whether to emit a metric when the rate limit is exceeded. The metric is fluentd_throttle_rate_limit_exceeded | ||
DESC | ||
config_param :group_emit_metrics, :bool, :default => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the name simple?
just enable_metrics should be fine.
lib/fluent/plugin/filter_throttle.rb
Outdated
@@ -68,12 +83,26 @@ def configure(conf) | |||
|
|||
raise "group_warning_delay_s must be >= 1" \ | |||
unless @group_warning_delay_s >= 1 | |||
|
|||
hostname = Socket.gethostname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the next few lines under the check if metrics enabled.
end | ||
|
||
def start | ||
super | ||
|
||
@counters = {} | ||
# TODO add more relevant metrics to throttling | ||
@metrics = {throttle_rate_limit_exceeded: get_counter(:fluentd_throttle_rate_limit_exceeded, "The exceeded rate of pods in the group")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be a variable. I dont think we need a map here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was made as a map just to make it standard to add more metrics. It can act as a template to add more metrics by adding in the map
Support to add prometheus metrics for observability for throttling of logs as requested here #15 . Opens more scope to add lot more metrics related to throttling. Corresponding tests are also added to the testing script and verified. Few plugin versions have been bumped up inorder to avoid few build issues.