Not sink the global metrics #23
Conversation
The global metric is controlled by the k6's ingester output, so the sink operation on the global metric in the sample is already executed. Use a local metric in the metricsStorage for executing a local and dedicated sink in the extension. It fixes the data race where both the extension and the k6 ingester calls Add and sink for the same metrics.
pkg/remotewrite/metrics.go Outdated
| // TODO: this is just avoiding duplicates with the previous | ||
| // Implement a better and complete solution | ||
| // maybe discard any timestamp < latest? | ||
| // | ||
| //current.Time = sample.Time // to avoid duplicates in timestamps |
There was a problem hiding this comment.
Just a note regarding solutions: can k6 guarantee to always send samples with timestamps in strictly increasing order?
current.Time = sample.Time // to avoid duplicates in timestampsThis assignment was mostly meant to avoid encountering an error on write (and losing data subsequently) and Prometheus bug mentioned below when such an error is thrown seemingly when it shouldn't be. Also, #11 for further investigation.
There was a problem hiding this comment.
I find just now the time for checking this comment. The Prometheus' timestamp generator function
that we are using for generating the timestamp has a millisecond precision that it could be very low if we consider our high concurrent world. If the remote write supports higher precision we could just use thesample.Time.UnixNano(), if not then we need to discard or wait for the new data modelling (hopefully available soon). WDYT? Could it explain the issue?
There was a problem hiding this comment.
Yes, I agree that improving precision could help with this particular problem but even if it's possible, I'm not sure about drawbacks it'll bring. E.g. do people actually need precision higher than ms? what else would it impact during processing? etc. Also, I believe precision question is valid for both Prometheus and k6 implementations, IIRC.
So IMO, precision is a multi-level problem here.
This change feels like a bit of an experiment to me 🙂 Definitely 👍 for making more experiments and figuring out when exactly duplicate timestamps happen.
There was a problem hiding this comment.
I removed all the commented lines and I'm going to merge this PR so we can fix the data race. We need to find the main issue about duplicates and fix it in a dedicated PR. I expect to address it by implementing the k6 metrics refactoring.
The specific hack is not required with the new local model. Regarding the original issue we need to get the real problem and apply a complete solution. We expect to fix with a PR for github.com//issues/11
Running the output with the xk6 race detector enabled a data race is caught:
The global metrics referenced in the samples are managed by the k6's ingester output, so the sink operation is already executed from it. So, executing concurrently the sink on the metric in the output generates a data race.
The fixes use new dedicated metrics in the
metricsStoragefor executing the sink locally in the extension.