The code itself looks okay, though I'm unsure about where exactly this is going. What are you planning on doing with this?

The code itself looks okay, though I'm unsure about where exactly this is going. What are you planning on doing with this?

So this change just extends the scrape loop to actually give a way to scrape and push the exemplars to an Appender.

So a user can now build a custom Appender against the new interface and use that to push metrics with exemplars to other backends by running the scrape loop. Given that the prom client now has experimental support for exemplars this is useful. The change as posted should be complete without any further followups.

This is separate from the other work around an exemplar store in prometheus and pushing exemplars through remote write (#6309), that was shelved till we have a better design..

Given that the prom client now has experimental support for exemplars this is useful. The change as posted should be complete without any further followups.

The change is not complete, it's only adding dead code. Having two interfaces is also a bit weird, which is why I'd like to see the rest of the change.

Hey @shreyassrivatsan, thanks for opening this PR. I think in general this change makes sense (alongside the rest of the code that would then store exemplars), but I think it might make more sense to have separate samples and exemplars Appender interfaces. That won't block one struct from implementing both, which is what I think you guys are wanting to do, but leaves things a bit cleaner from my perspective given that Prometheus won't be storing exemplars in anything long term for the moment.

I think we should probably coordinate between the two of us, or anyone else at Chronosphere who is doing exemplars work, to ensure we're not doing overlapping work. I only just pushed the change today, but in my WIP branch (pr here) I've got a very basic in memory implementation of a ExemplarAppender and also an ExemplarQueryable interface that I've hooked up to a handler for the web API. Curious if you have thoughts on that PR and I'd be happy for you to contribute at least the test from this PR to my branch! Let me know if you have some time to talk more next week.

Hey @shreyassrivatsan, thanks for opening this PR. I think in general this change makes sense (alongside the rest of the code that would then store exemplars), but I think it might make more sense to have separate samples and exemplars Appender interfaces. That won't block one struct from implementing both, which is what I think you guys are wanting to do, but leaves things a bit cleaner from my perspective given that Prometheus won't be storing exemplars in anything long term for the moment.

Yeah, I see the reasoning for not extending the Appender as I have done above..

But having separate interfaces even if they can be implemented by the same struct, makes it non-trivial to store exemplars along side the datapoint values if one wants to. I posted a comment on your diff describing the thought process a bit.. Let me know what you think..

Replying to your comments from both PR's here.

But having separate interfaces even if they can be implemented by the same struct, makes it non-trivial to store exemplars along side the datapoint values if one wants to.

The one concern I have with a separate ExemplarAppender interface is that it expects that exemplars are stored outside of the actual time series. Given that exemplars are emitted alongside a value in a time series, I think its beneficial to make the decision of how to persist them a concern of the storage.

The argument against this is that Prometheus doesn't have any immediate plans to store exemplars alongside sample data, nor return samples and exemplars as part of the data for a single query. I think for now we'd probably want to avoid adding noop's to TSDB but that's just my opinion.

By augmenting the appender interface with something like this #6844 (limitedly intrusive) it allows the storage to decide how it would like to deal with exemplars - by implementing the Appender accordingly. A storage can ignore the exemplar, store both exemplar and value or just store the exemplars like in the in-memory storage you have in this PR..

This is a good argument for your interface, assuming the TSDB maintainers aren't against those noop's.

I don't have a strong opinion either way; I personally would go with two separate interfaces as they're, in my mind, different data sets that need to be handled differently. Lets see if we can get opinions from anyone else on this.

I think for now we'd probably want to avoid adding noop's to TSDB but that's just my opinion.

It depends on how the rest of the code looks really. For example if the exemplar stuff was a wrapper on top of storage it'd make sense. If it's a completely different interface, it wouldn't.

I personally would go with two separate interfaces as they're, in my mind, different data sets that need to be handled differently.

The proposal in this PR isn't two separate interfaces, it's a sub and super interface - which feels weird. As I said we need to see where this is going, this PR is incomplete.

The proposal in this PR isn't two separate interfaces, it's a sub and super interface - which feels weird. As I said we need to see where this is going, this PR is incomplete.

You can see my proposed interface/implementation, which I linked to earlier, here. In that PR I have separate interfaces for samples (Storage) vs exemplars (ExemplarStorage).

In that PR I have separate interfaces for samples (Storage) vs exemplars (ExemplarStorage).

That appears as a valid approach to me.

The argument against this is that Prometheus doesn't have any immediate plans to store exemplars alongside sample data, nor return samples and exemplars as part of the data for a single query. I think for now we'd probably want to avoid adding noop's to TSDB but that's just my opinion.

Makes sense. Based on the above and the remaining comments on the PR I am going to go ahead and close this.