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

Send zeros for failure false and true #24

Closed
dirkjot opened this issue Nov 2, 2017 · 9 comments
Closed

Send zeros for failure false and true #24

dirkjot opened this issue Nov 2, 2017 · 9 comments
Assignees
Labels
Milestone

Comments

@dirkjot
Copy link

dirkjot commented Nov 2, 2017

I think the plugin needs to send zero values in some cases. Specifically, when the first observation (value) for metric{sampler_name="spam", failure="false"} is created, a matching observation metric{sampler_name="spam", failure="true"} 0 should be emitted.

Sending zero seems not a common thing in Prometheus, usually we send nothing to indicate that no observations have been made. That is correct for (for example) sampler_name because having observations on sampler spam does not imply that sampler ham exists.

However, assertion success and failure are related in the human mind: with a for assertions, we expect the total assertions a_t to be the sum of successes a_s and failures a_f. Unlike the samplers, having any number of successes implies that we also have failures (possibly 0), and vice versa.

Currently, the expectation is correct most of the time, iff a_s > 0 & a_f > 0. If no failures have been found yet: a_t != a_s + <undefined> The solution is to set a_f = 0 if a_s is set/modified and a_f has not been set yet.

Use case: I have a Grafana panel with the number of failures in my nightly tests, based on metric{failures="true"}. If all goes very well, the number shows up as "n/a" because no failures have been found. Switching to display the number of successes will go wrong when all tests fail.

@johrstrom
Copy link
Owner

+1. I'm sorry for not getting back around to this. The prometheus documentation clearly says that we should do this as well as an exporter. In the coming weeks I'm going to redouble my efforts on this library.

@johrstrom johrstrom self-assigned this Mar 23, 2018
@johrstrom johrstrom added the bug label Mar 27, 2018
@johrstrom
Copy link
Owner

This will be coming shortly in the 0.1.0 release. It's currently a very buggy branch at the moment, but the idea is that users will create whatever metrics they wish.

So the library itself won't dictate what metrics are created, users have full control on what metrics are created, their name, they're type and how they're updated.

@johrstrom johrstrom added this to the 1.0 milestone Feb 4, 2019
@johrstrom
Copy link
Owner

To be clear, the prometheus documentation on why this is important is here.

In 0.2.0-rc3 there are different metrics for total, success and failure. Perhaps it would be better to combine all this functionality into 1 'thing' from a UX perspective, because they are indeed actually facets of the same phenomena.

The proposal for such a metric would be something like this functionality: my_metric turns into my_metric_success, my_metric_failure and my_metric_total with the appropriate labels. So, say, the initial case where something is successful _successful{foo="bar"} = 1 _failure{foo="bar"} = 0 and _total{foo="bar"} = 1.

johrstrom added a commit that referenced this issue Mar 9, 2019
This adds a new metric type with the enum value SUCCESS_RATIO.  It's basially a counter for
success, failure and total all grouped together.  This way we'll initialize 0s for failures
when successes inc to 1 and vice versa.
johrstrom added a commit that referenced this issue Mar 11, 2019
@johrstrom
Copy link
Owner

@dirkjot I believe I've completed this functionality. I'm sorry it took so long. I've got it on this branch https://github.com/johrstrom/jmeter-prometheus-plugin/tree/dev and would like your feedback (or anyone's) if able.

@dirkjot
Copy link
Author

dirkjot commented Mar 12, 2019

I really like how you defined a new type, a much cleaner solution than I had in mind. The code changes are nicely isolated and everything looks excellent. I don't have a running Prometheus instance here so I cannot test it right now.

And by the way: Wow, that is amazing! It was always my plan to roll it out myself, but then I changed projects and life interfered. But I am sure a fair few people are waiting for this...

@johrstrom
Copy link
Owner

Thanks for the quick response!

I'm leaving all 4 use cases in the listener, just in case someone wants one individually.

  • counting success only
  • counting failure only
  • counting total
  • success_ratio which is all three of the above

I'll close the ticket when the feature gets released. Any other feedback on having all 4 use cases enabled is welcome too (by anyone).

@dirkjot
Copy link
Author

dirkjot commented Mar 12, 2019

I think that is the right thing to do: I have seen clients want any combination of the above.

My only other suggestion would be to point out in the docs that Prometheus is a metrics solution and inherently lossy. So when using it to monitor Jenkins state you may (in theory) miss an update, even though I have rarely seen this in practice. Only a solution made for eventing (like Kafka) would aim to guarantee zero missed updates. Like a high-quality lossy jpg image is often indistinguishable from a non-lossy png, but it is an inherently different solution.

@johrstrom
Copy link
Owner

This is in master and released in version 0.2.0. Thanks for the initiating the ticket and help along the way.

@dirkjot
Copy link
Author

dirkjot commented Mar 15, 2019

💯 👍 🌴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants