Switch log line to metric for detecting hash collisions #786
Conversation
When experiencing a high rate of hash collissions we're observing bad performance in the agent and we suspect the deluge of logs contributes to the problem.
andresgalindo-stripe
requested review from
calvin681,
Andyz26,
fdc-ntflx and
dtrager02
as code owners
andresgalindo-stripe
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
andresgalindo-stripe
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
| this.metrics = new Metrics.Builder() | ||
| .id(metricGroup) | ||
| .addCounter("numHashCollisions") | ||
| .build(); |
There was a problem hiding this comment.
I don' t think this is ever going to get registered. The underlying Router implementation has a getMetrics method that is used to register the metrics. You could override that here but then you would need to re-instantiate numEventsRouted and numEventsProcessed to make sure those metrics are still sent (and probably use a metricsGroup like the parent class to avoid a breaking change).
There was a problem hiding this comment.
i think you need to register with something like "MetricsRegistry.getInstance().registerAndGet"
There was a problem hiding this comment.
Gracias y'all, used the singleton to register the metrics!
| long hash = hashFunction.computeHash(connectionBytes); | ||
| if (ring.containsKey(hash)) { | ||
| logger.error("Hash collision when computing ring. {} hashed to a value already in the ring.", connectionId + "-" + i); | ||
| this.collisionsCounter.increment(); |
There was a problem hiding this comment.
I think it might still be nice to have logging for which connections are duplicates. Could we maybe track at the connection level (instead of the partition level) and emit one log line with all of the duplicate connections (not virtual nodes in the hash ring)
There was a problem hiding this comment.
Added a debug log that should come out in a structure like:
{
<hash>: [
0: <first connection with hash>
1..N: <colliding connections>
]
}
I chose a debug log that we'll have to explicitly opt into because I expect the structure to be huge and potentially cause this pausing like behavior we've been seeing today.
There was a problem hiding this comment.
Oh and toString() contains all the interesting bits of info we'd need, taken from AsyncConnection.java:
@Override
public String toString() {
return "AsyncConnection [host=" + host + ", port=" + port
+ ", groupId=" + groupId + ", slotId=" + slotId + ", id=" + id
+ "]";
}There was a problem hiding this comment.
Hm, I think toString will be called by the logger lib so I don't need to call it myself
andresgalindo-stripe
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
Context
When experiencing a high rate of hash collisions we're observing bad performance in the agent and we suspect the deluge of logs contributes to the problem.
We still need to investigate why we're getting hash collisions (more on that below) but want to try and remediate the bad performance we're seeing first.
Regarding the hash collisions we don't suspect theres anything wrong with xx3 but think the method is receiving a large number of duplicate connections but we're still investigating.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all tests