MP-Metrics: Request for Feedback on Counters / @Counted

[Heiko Rupp]

In MicroProfile-Metrics we have taken annotations from DropWizard metrics and also the semantics.

Which turns out is problematic in day to day use.

Just try to guess without looking at the docs what the value of the counter cFoo is after the invocation of foo()

@Counted(name="cFoo")

public void foo() { ... }

Anyway, we want to fix this for MP-Metrics 2.0 so we have created the following and want your feedback on which option you prefer going forward.

Counters...

want to resolve these 3 issues:

• unexpected default for monotonic

• not able to see accurate high water mark for non-monotonic counters

• ParallelCounter isn't a Counter from Prometheus perspective

Background: ( https://github.com/eclipse/microprofile-metrics/issues/203 )

• @Counted annotation and its behavior comes from DropWizard Metrics.

• @Counted has an optional parameter, monotonic, that controls the behavior of the counter

• Default is that monotonic=false.  People using @Counted without setting a value for monotonic expect the monotonic=true behavior.

   @Counted()

   public void foo() {}

   // counts the number of invocations of foo                    

   @Counted(monotonic=true)

   public void foo() {}

   // counts the number of concurrent executions of bar                

   @Counted(monotonic=false)

   public void bar() {}

How to address Counter issues?

option 1:

• Introduce new @HitCounted / @ParallelCounted annotations

• @HitCounted behaves like @Counted(monotonic=true)

• @ParallelCounted behaves like @Counted(monotonic=false)

• Introduce new HitCounter / ParallelCounter interfaces.

• ParallelCounter keeps track of high/low water mark for each of last 10 minutes

• Deprecate/Remove @Counted

       

   // counts the number of invocations of foo                    

   @HitCounted()

   public void foo() {}

   // counts the number of concurrent executions of bar                

   @ParallelCounted()

   public void bar() {}

   // deprecated/removed

   @Counted()

   public void zzz() {}

• Pros:

• Avoids confusion over monotonic default

• Tracks high/low water marks

• non-monotonic counters can be shown as Gauges in Prometheus output

   

• Cons:

• Requires people familiar with @Counted to change code to use @HitCounted / @ParallelCounted - breaks backward compatibility, but makes it very obvious where changes need to occur

• Introduces 2 new metric types to MetricsRegistry

• new concepts of ParallelCounter and HitCounter aren't entirely intuitive

• Option 1.1: deprecate @Counted and remove later

• Option 1.2: immediately remove @Counted

option 2:

• Make @Counted's monotonic flag be required (instead of optional)

• Introduce new ParallelCounter sub-interface of Counter to track high/low water mark for each of last 10 minutes

   // counts the number of invocations of foo  

   @Counted(monotonic=true)

   public void foo() {}

   // counts the number of concurrent executions of bar

   @Counted(monotonic=false)

   public void bar() {}

   // compile error - monotonic parameter is required

   @Counted()

   public void zzz() {}

• Pros:

• Familiar for Dropwizard Metrics developers or mpMetrics-1.1 developers

• Avoids confusion over monotonic default

• Tracks high/low water marks

• non-monotonic counters can be shown as Gauges in Prometheus output

• The only people that need to use ParallelCounter subclass are exporter developers

• Cons:

• Requires people familiar with @Counted to always set the monotonic flag (minor break to backward compatibility), which is noisy

• Note: if the monotonic flag was not set on pre-existing code and app is deployed into a MPM2.0 environment, it may throw errors at runtime

[Ondro Mihályi]

Hi Heiko,

I miss the following option:

option 3:

• leave the @Counted annotation as is but deprecate the monotonic parameter and change its default value to monotonic=true. This would work in the same way as the new @Hitcounted annotation proposed in option1 but would be less confusing and wouldn't introduce a new annotation
• if monotonic=false is specified, it would behave as @ConcurrentGauge, although this would still be deprecated
  
• introduce new @ConcurrentGauge (instead of ParallelCounter in option1) to count the number of concurrent invocations. I think this is a better name than ParallelCounter because
  
• it's not a counter at all, it behaves as a a gauge in fact
• the word concurrent fits better than parallel. Parallel indicates that the counter itself processes something in parallel, while concurrent indicates that it's related to something that's concurrent. I admit this argument is subjective :)

Similar to option 1, option3 can be used as:

// counts the number of invocations of foo (monotonic, never decreases)

   @Counted

   public void foo() {}

   // gives the number of concurrent executions of bar (arbitrary number)

   @ConcurrentGauge()

   public void bar() {}

   // deprecated

   @Counted(monotonic=false)

   public void zzz() {}

• Pros:
• all of the option1

• Avoids confusion over monotonic default
• Tracks high/low water marks
• non-monotonic counters can be shown as Gauges in Prometheus output
  

• most natural API for newcomers
• only 1 new annotation
• Cons:
• breaks backwards compatibility for non-monotonic counters, but doesn't break the code
• this can be fixed easily by forcing monotonic=false or by a vendor-specific switch to do this globally
• the spec can also require that the default for the monotonic option is configured via MP Config to enable switching to the old behavior without code changes to migrate older applications easily
  
• Introduces 1 new metric type to MetricsRegistry, but I would argue that the new type is in fact a normal Gauge, so possibly no new type needs to be added

--Ondro

[Heiko Rupp]

Ondro,

thanks for the suggestion.

We discussed something similar. And while @ConcurrentGauge instead of @ParallelCounter is a good one, I don't like the "silent" change of the default for monotonic in @Counted, as this is a change in behaviour, that may hit some people hard.

And then perhaps it is not as bad, as @Counted(monotonic=false) is not really useable today when the metrics are pulled by some outside client (other than code running in the method to find out how often it is called in parallel). 

[Ondro Mihályi]

Hi Heiko,

My argument for the breaking change in option 3 is that the MP Metrics spec is still pretty new and the case of using plain @Counted without arguments (with the default of monotonic=false) is very rare in real applications. For those who use it:

• it shouldn't be very painful to update their applications because the breaking change would come very soon after the 1.0 version was released
• most of such applications would be in active development
• the change doesn't break the code, it would only break some metrics if an app is not modified

So the impact on existing users and their application will be minimal. Especially if we highlight this breaking change in the release notes and in prominent places.

If we still care about decreasing the impact even more, we can specify a standard MP Config flag to keep the old behavior as long as we keep the deprecated option. This would help migrating to a new version of the spec with just a single system property.

--Ondro

ut 18. 9. 2018 o 15:58 'Heiko Rupp' via Eclipse MicroProfile <microp...@googlegroups.com> napísal(a):

--
You received this message because you are subscribed to a topic in the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/microprofile/IvrBfHsq4nI/unsubscribe.
To unsubscribe from this group and all its topics, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/b13c7462-285a-436a-a190-167ad7fa4aa4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Guillermo González de Agüero]

+1 for the Config property. Sounds a reasonable approach for cases like this.

Expanding on that idea, there might even be a "org.eclipse.microprofile.version_compatibility_mode" to target behavior of a specific MicroProfile version for all specs. That way, if MP 2.2 makes some BC breaks and I want to make sure that everything works the same as 2.1, I just need to set "org.eclipse.microprofile.version_compatibility_mode=2.1". I don't need to bother what specs actually broke BC.

All 2.X versions would be supported until 3.0. Individual specs should provide individual Config keys to restore behavior on specific features. BC breaks should continue to be as rare as they are now.

Regards,

Guillermo González de Agüero

You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.

To post to this group, send email to microp...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/CACZTZYW0rQz4a8ks9sv4jXNtqPQrXA_uFAmUfn-_VjwmOtznvg%40mail.gmail.com.

[Emily Jiang]

I think the impact of breaking BC is very minimal in this circumstances. This is a valid reason to introduce BC. I don't think it worth the effort to introduce a config just for this. If someone tries to go back to the previous behaviour, they can use

@Counted(monotonic=false)

In general, I like what Ondro proposed of keeping Counted. I am not sure about the name of ParallelGauage. Since Heiko thinks this new annotation is the subclass of Counted, it might be better called ParallelCounted, as Counted does not directly link to counter, as gauage can count as well.

We should make sure ParallelCount or whatever the new name is has the same default for monotonic.

By the way, I think Counted is a gauage already as it can go up and down. In Prometheus, counter can only go up.

Emily

[Ondro Mihályi]

Hi,

I realized that some annotations already provide multiple related metrics, not just a single value. What if the @Counted annotation provided 2 metrics instead of just 1, number of total invocations and number of current concurrent invocations? If we leave the monotonic parameter as obsolete and ignored, it wouldn't require any migration steps besides configuring metrics consumers to get the number of concurrent invocations from a metric with a different name in the JSON/Prometheus output.

So, formally, I propose option 4:

• leave the @Counted annotation as is but deprecate the monotonic parameter

• ignore the monotonic parameter and always create both "monotonic" and "non-monotonic" metrics (for total invocations and current concurrent invocations)
• add a new method into the Counter interface to give the number of con current invocations. The current getCount() would be the number of times the method inc() was called, the new method, e.g. getCurrent(), would be the difference between the number of invocations of inc() and dec()
• the output of the metrics endpoint would contain the current counter metric with the total number of invocations and an additional metric wit ha suffix _current or _concurrent which would actually be a gauge and would give the value returned by the new getCurrent() method

In code:

// counts the number of invocations of foo and also computes the number of concurrent invocations

   @Counted

   public void foo() {}

...

@Inject @Metric

Counter fooCount;

fooCount.inc();

fooCount.dec();

fooCount.getValue(); // == 1

fooCount.getCurrent(); // == 0

...

In JSON:

    {
        "fooCount": 1, // this is the counter that is generated now

        "fooCount.current": 0   // this is a new gauge with the .current suffix

    }

Pros:

• the simplest solution, no new annotation or interface
  
• no breaking change in the application if annotation is used, only in the metrics output
• avoids confusion between monotonous and non-monotonous in the code
• allows deciding which metric should be consumed after an app is coded
• of course, all of the Pros from option 1:
  

• Avoids confusion over monotonic default
• Tracks high/low water marks
• non-monotonic counters can be shown as Gauges in Prometheus output
  

Cons:

• counting both metrics could add some overhead but it's negligible
• metrics output for non-monotonous counters isn't backwards compatible, but it's not very relevant as metrics consumers can be easily reconfigured
  
• in a rare case, thsi would require refactoring when using Counter interface for getting a value of a non-monotonous counter (gauge) - the new method getCurrent() would be used instead of getValue(). In other cases the code or behavior shouldn't break

Opinions?

--Ondro

ut 18. 9. 2018 o 22:54 'Emily Jiang' via Eclipse MicroProfile <microp...@googlegroups.com> napísal(a):

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/961507c6-d19e-44a6-a5cc-a73b1c8d1ac1%40googlegroups.com.

[Heiko Rupp]

Am Mittwoch, 19. September 2018 00:33:05 UTC+2 schrieb Ondro Mihályi:

Hi,

I realized that some annotations already provide multiple related metrics, not just a single value. What if the @Counted annotation provided 2 metrics instead of just 1, number of total invocations and number of current concurrent invocations? If we leave the monotonic parameter as obsolete and ignored, it wouldn't require any migration steps besides configuring metrics consumers to get the number of concurrent invocations from a metric with a different name in the JSON/Prometheus output.

Hm. I see the benefits here.

Now the original description also mentions a different issue: if you use @Counted(), then the counter value is incremented on entering a method and decreased when exiting. This means that from an external observer that is calling in every 30s you will not often see a value > 0, as this highly depends on the "correct timing". 

So I also proposed that in the case of monotonic=false, the metric would keep a number of buckets for the highest value during the bucket duration. We called that high-water-mark.

Those high water marks would not make (too) much sense in my opinion for the monotonic=true case. Surely you can determine the increase of the value per bucket-length, but that can easily be done by polling the counter more often from the external client.

[Ondro Mihályi]

A.d. watermarks, does it make sense to you to collect all this information using a single metric?

E.g. putting a @Counted annotation, it would collect all the following:

• number of invocations (how many times the method was entered) - with no suffix, with the same name as now in the JSON/Prometheus output
  
• number of concurrent invocations (difference between count of entered and exited) - with a suffix e.g. _current
  
• highest number of concurrent invocations - with a suffix e.g. _max
  

Does any other metric make sense?

With this, the only breaking change would be that the name of a non-monotonic metric would have the _current suffix, therefore metrics consumers would need to be reconfigured. No changes in teh code, except that the monotonic argument of @Counted would become obsolete and ignored.

--Ondro

[donbo...@gmail.com]

I like the new idea you're describing in option 4, Ondro.

In our original thinking on this we were considering having multiple maximums, one for each of the preceding completed 10 minutes, as metrics.  If we are automatically going to add the non-monotonic metric, I think we'd want to be more conservative about how many metrics to automatically add.

The problem I see with having a *_max metric, as it's currently described, is that it never resets (unless you restart your server).  That's how the idea for tracking the max for each of the previous 10 minutes came about.

Perhaps, rather than keeping the max for each of the last 10 minutes, we could keep just a single max representing the highest seen value in the last completed minute.  eg. at 8:45am, the _max metric would represent the highest value between 8:44:00 and 8:44:59.  In that case you would have...

• number of invocations (how many times the method was entered) - with no suffix, with the same name as now in the JSON/Prometheus output
  
• number of concurrent invocations (difference between count of entered and exited) - with a suffix e.g. _current
  

• highest number of concurrent invocations in the last complete minute - with a suffix e.g. _max
  

Assuming your metrics scraper runs at least once a minute you could have a complete picture with just one _max metric.

Don

[donbo...@gmail.com]

We had further discussion about this in our meetup this week and in gitter.  Having debated some of the previous ideas we have an Option 5.

option 5:

    - leave the @Counted annotation as is but REMOVE the monotonic parameter

        - counters will always only increment (never decrement - following prometheus convention)

        - change Counter interface to REMOVE dec() methods

    - add @ConcurrentGauge annotation to handle @Counted(monotonic=false) use case

        - @ConcurrentGauge would be implemented as a type/ctor/method interceptor that creates 3 gauge metrics

            - number of concurrent invocations (difference between count of entered and exited) - with a suffix e.g. _current

            - highest number of concurrent invocations in the last complete minute - with a suffix e.g. _max

            - lowest number of concurrent invocations in the last complete minute - with a suffix e.g. _min

    - add a ConcurrentGauge interface, which is returned when appropriate from MetricRegistry.getGauges()

            Gauge

                getValue()

            ConcurrentGauge extends Gauge

                getMax()

                getMin()

    - typical use case for @ConcurrentGauge would be to instrument a servlet doGet method -- to keep track of how many concurrent threads are running in that servlet.

    

    - as this breaks backwards compatibility this would first be included in MP Metrics 2.0

Comments?

[Heiko Rupp]

This is now Pull-Request First cut at new counter handling.

[Ondro Mihályi]

Thanks, Heiko.

I've reviewed it, looks good to me. We also need to update the changelog to inform people about the breaking changes and how to update their apps to use the new version of the spec.

ut 20. 11. 2018 o 17:27 'Heiko Rupp' via Eclipse MicroProfile <microp...@googlegroups.com> napísal(a):

--

You received this message because you are subscribed to a topic in the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/microprofile/IvrBfHsq4nI/unsubscribe.
To unsubscribe from this group and all its topics, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/6d4a38cc-07ae-48c4-a741-c23442f00293%40googlegroups.com.