Discussion, improve CounterDataPoint.inc() critical path

[cruftex]

@fstab, @dhoard, @tomwilkie, @zeitlinger

Thanks for the cool library. Its a pleasure to work with!

I was using it for a new project and started with placing own LongAdder in the code and expose via the collect() method. This way, I would I could manipulate it directly in the class implementing the service resulting in maximum performance. The code started to be come repetitive and add some point I realised that I am starting to implement something like CounterDataPoint again. To avoid this, I started digging into the code to see what the overhead would be.

With a code like counter.labelValues().inc(1) there is a bit of indirection until we hit LongAdder. However, there is already a faster path as the documentation in CounterDataPoint describes:

CounterDataPoint pendingTasks = counter.labelValues("pending");
pendingTasks.inc();

Looking deeper into the code there is a tiny bit more indirection, which could be avoided.

This patch is returning a specialised class in case the exemplars support is not enabled. The intended usage is:

    Counter counter =
      Counter.builder()
        .name("count_total")
        .withoutExemplars()
        .build();
    Counter.DataPointIgnoringExemplars fasterCounter = (Counter.DataPointIgnoringExemplars)
      counter.labelValues();
    fasterCounter.inc(1);

The unit test run successfully, however I realise that its breaking with the concept that the configuration can override what is configured in the code. I am not sure whether this would be tolerable for that special case. If an SRE enables exemplars for further diagnosis, the application would crash completely. Not good. However, probably

The patch is still worth discussion and worth applying without propagating to use Counter.DataPointIgnoringExemplars class directly. This class can be private to avoid wrong usage. Which brings me to the class visibility of Counter.DataPoint.

My IDE marks code with:

The problem is that it's used as type parameter in the main Counter class and therefor becomes visible. My recommendation would be to make this public, deprecate the interface CounterDataPoint and mandate to use Counter.DataPoint instead. That is painful and probably will not happen fast, however, it is a mismatch, since Counter is a class that is not open for extension and the only one that will ever provide a CounterDataPoint.

The patch also removes two unnecessary boolean fields. If that makes sense, I can also extract that as a housekeeping PR.

Thoughts?

[cruftex]

@fstab
Hmm.... According to the code comment actually withoutExemplars() should turn it off. Then I would not expect that it can turned on by the configuration again. I did another update, so that for the counter exemplars flag the code always takes precedence. Maybe that should be the case for all metrics?

    /** Allow Exemplars for this metric. */
    public B withExemplars() {
      this.exemplarsEnabled = true;
      return self();
    }

    /** Turn off Exemplars for this metric. */
    public B withoutExemplars() {
      this.exemplarsEnabled = false;
      return self();
    }

[zeitlinger]

@fstab Hmm.... According to the code comment actually withoutExemplars() should turn it off. Then I would not expect that it can turned on by the configuration again.

yes, I agree

[zeitlinger]

Maybe that should be the case for all metrics?

I think that's a great idea 😄

[zeitlinger]

however I realise that its breaking with the concept that the configuration can override what is configured in the code

there's also the concept that you should be able to get the best possible performance with this library 😄

[cruftex]

@zeitlinger
Thanks for the feedback. I am happy to have a look at the other metrics regarding turning of the exemplar support as well and do a PR.

What about deprecating CountDataPoint in favor of Count.DataPoint? I know that is a bit invasive suggestion. But the JavaDoc in CountDataPoint needs somehow to be addressed also.

[zeitlinger]

What about deprecating CountDataPoint in favor of Count.DataPoint? I know that is a bit invasive suggestion. But the JavaDoc in CountDataPoint needs somehow to be addressed also.

yes, sounds like the right way forward 😄

[zeitlinger]

Suggested change

	if (exemplarsEnabled && notTurnedOffWithinCode) {
	if (exemplarsEnabled && !Objects.equals(builder.exemplarsEnabled, false)) {

[zeitlinger]

builder can't be null (adding NullAway is on my list...)

[cruftex]

I think its better to address the withoutExemplars first and separately:

#1477

Regarding the counter optimization, I think this code looks rather ugly and hurts a lot:

    Counter counter =
      Counter.builder()
        .name("count_total")
        .withoutExemplars()
        .build();
    Counter.DataPointIgnoringExemplars fasterCounter = (Counter.DataPointIgnoringExemplars)
      counter.labelValues();
    fasterCounter.inc(1);

Better idea:

   HotCounter counter =
      HotCounter.builder()
        .name("count_total")
        .build();
   HotCounter.DataPoint fasterCounter = counter.labelValues();
    fasterCounter.inc(1);

While

Counter.builder()
        .name("count_total")
        .withoutExemplars()
        .build

Would also return a HotCounter.

[zeitlinger]

... Would also return a HotCounter.

sounds good 😄