Hono: AMQP metrics report negative number of authenticated connections

[bordeuax]

Periodically(I would say rather sporadically !) AMQP adapter report a negative metric number for authenticated connections. Unfortunately, for a while I tried to find the cause and reproduce this but without success. But anyway, when I took a look at MicrometerBasedMetrics.java, I would like to propose to have more _safe_ incremental and detrimental operations for connections counters, something like this:

private void tryToDecrement(AtomicLong longValue) {
   longValue.getAndUpdate(oldValue -> oldValue > 0 ? oldValue - 1 : 0);
}

private void tryToIncrement(AtomicLong longValue)
 {
  longValue.getAndUpdate(oldValue -> {
   if (oldValue == Long.MAX_VALUE) {
    throw new IllegalStateException("Long value overflow!");
   }
   return oldValue + 1L;
  });
 }
}

[kaniyan]

I don't think this is just due to long overflow. If it is due to number overflow, then it could happen when more than 9,223,372,036,854,775,807 devices are simultaneous connected to a single _AMQP_ adapter instance. And I don't think it's the scenario we are facing here.

These methods to increment and decrement connections in MicrometerBasedMetrics are not only being used by the _AMQP_ adapter but also by the _MQTT_ adapter. And this issue regarding negative number of connections is reported only in the _AMQP_ adapter and the problem could lie some where else. As I remember, the _MQTT_ adapter already underwent load tests with huge number of devices and if this is the case, we should have noticed there.

[bordeuax]

@kaniyan , is nothing related to long overflow, is just a proposal to have a _safe_ decrement and increment operation for something that logical can't be less than zero. You are right that the problem could be some where else and with this proposal we just some how mask the real problem and not fixing it(unfortunately i didn't found the exact behavior that can generate this bug :( ). I want to mention 2 things; 1st in Prometheus we can see very clear that this negative number have been fetched from adapter and the 2nd (that is very important for me!) based on this data we have the connection resource limits that is calculated on wrong base. Perfectly will be to fix this in the code and also why not to have additional mechanism for _safe_ increment/decrement.

[fkaltner]

I am able to reproduce this on my machine by setting the AMQP / MQTT adapter's connection limit to 1 and connecting 2 clients.

When I connect the second client, I see negative values in the Prometheus dashboard.

I'd like to continue to investigate this, @sophokles73 can you assign this issue to me?

[sophokles73]

@fkaltner sure, be my guest :+1:

[fkaltner]

I think I found the cause:

In the AMQP adapter the connection count was incremented when opening a new connection _after_ checking if the adapter's connection limit was hit.
The decrement however was done in any case, since it is part of the connection's close handler.

As you can see in e70476c I fixed this by moving the increment of the connection count to the connection's open handler.

Nevertheless I am not sure of any side-effects. @kaniyan / @sophokles73 WDYT?

[sophokles73]

@fkaltner This sounds reasonable. The proton-vertx API is misleading here as it suggests that a server could reject a connection by means of invoking the ProtonConnection.close() method instead of open(). However, in AMQP 1.0 there is no such thing as rejecting a connection request. Instead, the server always sends its open frame followed by an immediate close frame. So the connection will always be established in any case. However, if the resource limits check fails, the server immediately closes the connection again.
So, indeed, moving the code for incrementing the counter to the open handler should fix this issue in the right spot FMPOV :+1:
Can you create a PR including a unit test?

[fkaltner]

@sophokles73 Yep, just created a PR. Thanks for the insights into the AMQP protocol, I also adapted the comment in the unit test to reflect your explanation.