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.

Sign up for GitHub

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

Jump to bottom

Add extra telemetry to monitor failures #660

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

Add extra telemetry to monitor failures #660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions policy/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"sync"
"time"

metrics "github.com/armon/go-metrics"
"github.com/google/go-cmp/cmp"
hclog "github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-multierror"
Expand Down Expand Up @@ -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 •
edited
Loading

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 🙂

h.log.Warn("failed to get target status", "error", err)
return nil, err
}
Expand Down
1 change: 1 addition & 0 deletions policy/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func (m *Manager) monitorPolicies(ctx context.Context, evalCh chan<- *sdk.Scalin

case err := <-m.policyIDsErrCh:
m.log.Error("encountered an error monitoring policy IDs", "error", err)
metrics.IncrCounter([]string{"policy", "monitor", "failure_count"}, 1)
if isUnrecoverableError(err) {
return err
}
Expand Down
3 changes: 3 additions & 0 deletions policyeval/base_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ func (w *BaseWorker) handlePolicy(ctx context.Context, eval *sdk.ScalingEvaluati
"on_check_error", eval.Policy.OnCheckError,
"error", err)

metrics.IncrCounterWithLabels([]string{"policy", "run_check", "failure_count"}, 1, []metrics.Label{{Name: "check", Value: checkEval.Check.Name}})

// Define how to handle error.
// Use check behaviour if set or fail iff the policy is set to fail.
switch checkEval.Check.OnError {
Expand Down Expand Up @@ -287,6 +289,7 @@ func (w *BaseWorker) handlePolicy(ctx context.Context, eval *sdk.ScalingEvaluati

err = w.scaleTarget(logger, target, eval.Policy, *winner.action, currentStatus)
if err != nil {
metrics.IncrCounter([]string{"target", "scale", "failure_count"}, 1)
return err
}

Expand Down