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 alert template expanding failure metric #4747

Conversation

@mucahitkurt
Copy link
Contributor

@mucahitkurt mucahitkurt commented Oct 16, 2018

@fabxc

Closes #4634

@mucahitkurt mucahitkurt changed the title add alert template expanding failure metric #4634 add alert template expanding failure metric Oct 16, 2018
Copy link
Member

@simonpasquier simonpasquier left a comment

Thanks for tackling this! We usually don't unit test the metrics.

@@ -34,6 +35,17 @@ import (
"github.com/prometheus/prometheus/util/strutil"
)

var (
expandFailures = prometheus.NewCounter(prometheus.CounterOpts{
Copy link
Member

@simonpasquier simonpasquier Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good practice to have also a counter on the overall number of expansions (irrespective if they fail or not) which would be prometheus_template_expansions_total.

@@ -34,6 +35,17 @@ import (
"github.com/prometheus/prometheus/util/strutil"
)

var (
expandFailures = prometheus.NewCounter(prometheus.CounterOpts{
Name: "prometheus_template_expand_failures_total",
Copy link
Member

@simonpasquier simonpasquier Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prometheus_template_expansion_failures_total rather?

@@ -271,18 +283,21 @@ func (te Expander) Expand() (result string, resultErr error) {
var ok bool
resultErr, ok = r.(error)
if !ok {
expandFailures.Inc()
Copy link
Member

@simonpasquier simonpasquier Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the other expandFailures.Inc() if you move add this at the end of the defer function

if resultErr != nil {
  expandFailures.Inc()
}

prometheus_template_expansions_total metric is added
prometheus_template_expand_failures_total metric is renamed to prometheus_template_expansion_failures_total
additional unit test code for metrics is deleted
unnecessary metric increase code is refactored

Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>
@mucahitkurt
Copy link
Contributor Author

@mucahitkurt mucahitkurt commented Oct 16, 2018

@simonpasquier thank you for the review. I've fixed the all.

Copy link
Member

@simonpasquier simonpasquier left a comment

one small nit.

@@ -18,6 +18,7 @@ import (
"context"
"errors"
"fmt"
"github.com/prometheus/client_golang/prometheus"
Copy link
Member

@simonpasquier simonpasquier Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go at L34

Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>
@mucahitkurt
Copy link
Contributor Author

@mucahitkurt mucahitkurt commented Oct 23, 2018

Hi @simonpasquier ,
Is this PR OK. for you?

@simonpasquier
Copy link
Member

@simonpasquier simonpasquier commented Oct 23, 2018

LGTM. cc @brian-brazil

})
expansionTotal = prometheus.NewCounter(prometheus.CounterOpts{
Name: "prometheus_template_expansions_total",
Help: "The total number of template expansions.",
Copy link
Contributor

@brian-brazil brian-brazil Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite true as it's excluding HTML expansions. The name could be clearer.

Copy link
Contributor Author

@mucahitkurt mucahitkurt Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to textTemplateExpansionFailures and textTemplateExpansionTotal

mucahitkurt added 2 commits Oct 24, 2018
…ansionTotal and textTemplateExpansionFailures

Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>
"github.com/prometheus/prometheus/promql"
"github.com/prometheus/prometheus/util/strutil"
)

var (
textTemplateExpansionFailures = prometheus.NewCounter(prometheus.CounterOpts{
Name: "prometheus_text_template_expansion_failures_total",
Copy link
Contributor

@brian-brazil brian-brazil Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

template_text

This is the template subsystem.

Copy link
Contributor Author

@mucahitkurt mucahitkurt Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, because of the import statement, text_template "text/template", I renamed to this name. I'll rename to prometheus_template_text_expansion_failures_total and prometheus_template_text_expansions_total.

Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>
@mucahitkurt
Copy link
Contributor Author

@mucahitkurt mucahitkurt commented Oct 31, 2018

@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Oct 31, 2018

I'm waiting for 2.5 to be out before merging this, as we're in a freeze right now.

@brian-brazil brian-brazil merged commit 4a6c329 into prometheus:master Nov 6, 2018
4 checks passed
@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Nov 6, 2018

Thanks!

@mucahitkurt mucahitkurt deleted the enhancement/addTemplateExpandingFailureMetric branch Nov 7, 2018
@simonpasquier simonpasquier added this to the v2.6.0 milestone Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants