What way do you want all of this reviewed? Everything in this PR?

Also the exemplars have to go before the samples in the WAL, otherwise you can't tell if a sample at the very end of the WAL has an exemplar or not.

I'd prefer to prioritize merging #6635, but Ganesh specifically asked for the WAL and remote write changes in a separate PR so he could review any changes to TSDB code more easily. If you want to review this PR then you only need to look at the most recent commit. Once #6635 is merged I'll rebase obviously.

Also the exemplars have to go before the samples in the WAL, otherwise you can't tell if a sample at the very end of the WAL has an exemplar or not.

Can you elaborate here, why is this a requirement and where is the code that says we can't match up a sample at the end of a segment and an exemplar at the beginning of a segment?

Nothing I'm doing requires explicit matching of an exemplar to a sample in the WAL.

Grand, is #6635 ready for review? You still seemed to be working on it.

Can you elaborate here, why is this a requirement and where is the code that says we can't match up a sample at the end of a segment and an exemplar at the beginning of a segment?

With the current code, if the last entry in the WAL is a sample how do you know that you've now read everything from the WAL about that sample?

Nothing I'm doing requires explicit matching of an exemplar to a sample in the WAL.

Just because you don't need it right now doesn't mean we should make it impossible for someone else who wants it in future. It's only reordering some code. The reason why this is important will be more apparent with metadata, but the same applies here so we should be consistent.

Grand, is #6635 ready for review? You still seemed to be working on it.

There's certainly some work still left, but I think it's ready for another review. I've also responded to any unresolved comments.

With the current code, if the last entry in the WAL is a sample how do you know that you've now read everything from the WAL about that sample?

Just because you don't need it right now doesn't mean we should make it impossible for someone else who wants it in future. It's only reordering some code. The reason why this is important will be more apparent with metadata, but the same applies here so we should be consistent.

The exemplar would at least have the scrape timestamp for lining it up with the sample, but I see how that could get messy, especially with the addition of metadata. I can see the reasoning behind reordering now since we would want to avoid having to change the WAL format in the future. I was mostly wondering if there was a use case or current requirement that I was breaking by not writing the exemplar record before the sample record, but seems that's not the case.

There's certainly some work still left, but I think it's ready for another review. I've also responded to any unresolved comments.

I'll see about getting to that this week then.

I was mostly wondering if there was a use case or current requirement that I was breaking by not writing the exemplar record before the sample record, but seems that's not the case.

Not specifically, but it may come up when I get to reviewing this PR. I believe I had made at least one comment touching on this general area - and taking a quick peek at this PR the question of how a exemplar is tied to a specific sample remains unresolved.

I notice that the api returns

{
  "status": "success",
  "data": null
}

The query api does not return null, but returns [], which I find better.

@codesome if you could have another look at the WAL code and format documentation and just let me know which open comments you think still need to be dealt with. I think there was initially some confusion around what the exemplar labels were (vs series labels).

Regarding Can we get rid of this first row? The first exemplar could have the full ID and the timestamp, and remaining examplars to have the delta. , my question would be is there a reason you think we should change the format that way, or does it just seem like the first row is unnecessary? I just based the exemplars records on samples records, with the addition of the exemplars label set. I don't have any context for why those records were formatted that way vs what you're proposing.

@csmarchbanks let me know what you think of the latest commit, bit a refactor to include RW metrics for exemplars in a more correct way, instead of having some exemplars specific metrics and others that were named with samples but included both.

how bad would be if we would add exemplar directly to the sample (as optional) thing, in proto?

There are pros and cons for both from what I see.

Depending on how the remote storage is setup, sending samples and exemplars in separate requests might be beneficial if exemplar storage could become a bottleneck for samples (but splitting that request could be taken care at remote storage too, but will it affect the write SLOs?). (that said, irrespective of that, I think Prometheus itself will wait before sending more samples if exemplar is taking time to preserve the order in a series?)

And if we send samples and exemplars in same request, we could save some network bandwidth and costs by not duplicating the series labels :)

Given the flexibility of remote storage, I feel having exemplar with the samples is better.