Ansible add metrics #5438
Ansible add metrics #5438
Conversation
openshift-ci
bot
added
the
needs-rebase
asmacdo
added
the
do-not-merge/work-in-progress
asmacdo
force-pushed
the
ansible-add-metrics
branch
from
c8c1b15 to
dbb8434
Compare
openshift-ci
bot
removed
the
needs-rebase
asmacdo
force-pushed
the
ansible-add-metrics
branch
from
dbb8434 to
545da23
Compare
Metrics are created with the osdk_metrics module in operator-sdk-ansible-util which communicates with an ansible API server. Signed-off-by: austin <[email protected]>
asmacdo
force-pushed
the
ansible-add-metrics
branch
from
545da23 to
a2e6412
Compare
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
There was a problem hiding this comment.
The only change that I think really needs to be made is un-go-routining the server function and letting the caller decide to run it. Everything else is refactoring and style things to make the function more clear/readable. I think at least you should break the main HandleUserMetric function into some helper functions to make it more self-documenting.
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
asmacdo
force-pushed
the
ansible-add-metrics
branch
from
bdcc650 to
90f0035
Compare
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
| switch v := collector.(type) { | ||
| // Gauge must be first, because a Counter is a Gauge, but a Gauge is not a Counter. | ||
| case prometheus.Gauge: | ||
| if err := handleGauge(metricSpec, v); err != nil { | ||
| return err | ||
| } | ||
| case prometheus.Counter: | ||
| if err := handleCounter(metricSpec, v); err != nil { | ||
| return err | ||
| } | ||
| // Histogram and Summary interfaces are identical, so we accept either case. | ||
| case prometheus.Histogram: | ||
| if err := handleSummaryOrHistogram(metricSpec, v); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of the type switch, you can do:
switch {
case metricSpec.Gauge != nil:
// gauge stuff
case metricSpec.Counter != nil:
// counter stuff
// etc
}Not a blocker for this PR, but would allow you to split the summary and histogram helper functions.
There was a problem hiding this comment.
I dont think that will work in edge cases where the user is erroneously mixing types. For example, if the user creates a metric "my_counter_metric", and then calls the gauge function on "my_counter_metric". In this case, the current code will see that it is a counter, and handle it as a counter, and send a failure message that "you cant change counter to gauge". For your suggestion, it will attempt to handle it as a gauge, which could cause silent problems I think?
There was a problem hiding this comment.
Gotcha, do we validate anywhere else that for existing metrics, the type doesn't change?
There was a problem hiding this comment.
We didn't collect custom metrics from users, so AFAIK it hasn't come up before.
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
Description of the change:
Motivation for the change:
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs