Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

the-nando
Copy link

@the-nando the-nando commented Jul 9, 2023

Fixes: #661

@the-nando the-nando requested review from jrasell and lgfa29 as code owners July 9, 2023 08:17
@the-nando the-nando force-pushed the feature/extra-telemetry branch from d14cf2a to 4d5e69e Compare July 9, 2023 08:43
@the-nando the-nando force-pushed the feature/extra-telemetry branch from 4d5e69e to a632034 Compare July 9, 2023 08:45
@@ -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)
Copy link
Contributor

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)

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Author

@the-nando the-nando Jul 26, 2023

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.

Copy link
Contributor

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 🙂

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.

Extra telemetry on policy evaluation failure
3 participants