pkg/promtail/client: Handle fingerprint hash collisions

[pstibrany]

What this PR does / why we need it:
Due to fingerprint hash collisions, Promtail client can mix entries for different labels into the same stream. This can lead to having entries with incorrect labels, and possible out of order errors.

Which issue(s) this PR fixes:
Not sure if there is issue for this.

Special notes for your reviewer:
I've used large prime number to find next possible fingerprint value. Any prime (except 2) would work here, as would simple increment. I wanted to avoid using next fingerprint (+1), because fast fingerprint creates similar values for similar labels.

Checklist

• Documentation added (no extra documentation)
• Tests updated

[pracucci]

The solution is pretty neat!

We have the same issue in pkg/logentry/stages/timestamp.go, but it may be a bit more more complicated to solve there using the same approach because mappings would last forever. Maybe we should see the performance penalty of using the slow fingerprinting there, before taking an informed decision?

[pstibrany]

The solution is pretty neat!

We have the same issue in pkg/logentry/stages/timestamp.go, but it may be a bit more more complicated to solve there using the same approach because mappings would last forever. Maybe we should see the performance penalty of using the slow fingerprinting there, before taking an informed decision?

Thanks for taking your time for review. I wouldn’t be worried about “slow” fingerprint, but despite using it, there may still be collisions.

[pstibrany]

We have the same issue in pkg/logentry/stages/timestamp.go ...

I think correct solution there is to implement similar remapping on collisions, and expire entries from lastKnownTimestamps and remapping map after some period of time since last use (say 30mins).

[pstibrany]

I think correct solution there is to implement similar remapping on collisions, and expire entries from lastKnownTimestamps and remapping map after some period of time since last use (say 30mins).

Ah, I can see we already use golang-lru cache there. It can be configured with eviction handler, which would allow us to remove collision mapping entry as well.

[pstibrany]

I've updated PR to not use fast fingerprint in promtail. We use labelset.String as map keys instead. While this produces extra garbage in promtail, it doesn't reinvent hash maps, keeps code simple and solves the collision problem.

[pracucci]

I do prefer this last approach, being easier and more generally applicable. I don't see memory consumption issues in the Promtail client (because the batch lifecycle is short), while in the timestamp stage we do limit the last known timestamp to 10K entries: assuming a worst case scenario of 1KB each log stream label set, it's 10MB memory consumption, which doesn't look prohibitive to me.

[pstibrany]

... assuming a worst case scenario of 1KB each log stream label set, it's 10MB memory consumption, which doesn't look prohibitive to me.

Thanks for your review! My concern is more about calling labelset.String() on every single log line. Not so much memory consumption, but memory allocation rate. That said, let's optimize it when it becomes a real problem.

[cyriltovena]

LGTM