-
Notifications
You must be signed in to change notification settings - Fork 83
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
Add extra telemetry to monitor failures #660
base: main
Are you sure you want to change the base?
Conversation
d14cf2a
to
4d5e69e
Compare
4d5e69e
to
a632034
Compare
@@ -228,6 +229,7 @@ func (h *Handler) handleTick(ctx context.Context, policy *sdk.ScalingPolicy) (*s | |||
|
|||
status, err := target.Status(policy.Target.Config) | |||
if err != nil { | |||
metrics.IncrCounter([]string{"policy", "target_status", "failure_count"}, 1) |
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.
We have a standard set of metrics already of these kind of plugin invocations:
https://developer.hashicorp.com/nomad/tools/autoscaling/telemetry#scaling-metrics
But it's only for a few actions, so it would good to expand to others as well.
Although having to remember to emit these metrics all the time is kind of annoying and easy to forget, so I think we can do something better.
Instead of leaving it to the caller to remember to handle this, the way I'm thinking about is to have the plugin manager return an instance of the requested plugin wrapped in a struct that handles the metric.
Here is a quick prototype of how this would look like for a target plugin: 4340235
This way, emitting the metrics is completely transparent to callers and we can ensure the metrics are always being handle.
What do you think of this approach?
(cc @Juanadelacuesta and @jrasell as you've been digging into the autoscaler recently)
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.
@lgfa29 that approach makes sense to me and acts like a middleware/wrapper which should make plugin developer life easier.
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.
Thanks for the extra input 😄
@the-nando do you think you would be able to implement something like 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.
Thanks @lgfa29 for the input, much better and cleaner than my hacky attempt at wiring metrics from within the plugin.
I'll take a stab at it in a week or so.
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.
Thank you! Feel free to reach out if you need any help 🙂
Fixes: #661