exemplar querying #4181

cstyan · 2021-05-13T04:47:24Z

Still needs a few more tests, but the implementation should be good for a first review.

Signed-off-by: Callum Styan callumstyan@gmail.com

Signed-off-by: Callum Styan <callumstyan@gmail.com>

mdisibio · 2021-05-13T13:25:37Z

pkg/distributor/query.go

+			result = append(result, b[j])
+			j++
+		} else {
+			result = append(result, a[i])


I'm not sure, but should this compare Value as well? The Prometheus dedupe logic does so I think different exemplars with the same Ts but different Values are possible. https://github.com/prometheus/prometheus/blob/main/pkg/exemplar/exemplar.go#L49

My understanding is that any data that comes into cortex is replicated to the right ingester replica set, and the write is successful when we're replicated to a quorum (2 out of 3 in our case).

So my assumption is that while not every ingester in the replica set would have every exemplar, there shouldn't be any ingester that has an exemplar at a certain timestamp that doesn't exist in another ingester. That is, because we're not scraping and (potentially) setting the timestamps ourselves within cortex, merging by timestamp should be enough.

Anything I'm missing here?

So my assumption is that while not every ingester in the replica set would have every exemplar, there shouldn't be any ingester that has an exemplar at a certain timestamp that doesn't exist in another ingester.

Think about this edge case:

Remote write exemplar for series X with TS=1 and V=1. Replication set is ingesters 1,2,3. This write succeed towards ingester 1 and 2, but not 3 (not ingested here).

Remote write exemplar for series X with TS=1 and V=2. Replication set is ingesters 1,2,3. This write succeed toward ingester 3 (was not ingested before) but fails on 1 and 2 (because of same timestamp but different value). Even if quorum has not been reached, the exemplar with V=2 is written anyway to ingester 3 (will not rollback from ingester 3 because writing to 1 and 2 has failed).

When you read back, you will have two exemplars with TS=1 and values 1 and 2.

The current implementation picks a random exemplar (between the two) if the timestamp is the same, without checking if value is equal as well. I personally think it's fine, but I wanted to outline the edge case above so you can take an informed decision.

How does Cortex handle this situation for samples? I think exemplars are handled the same way here since this is essentially a copy of util.MergeSampleSets.

The only way this edge case could really happen that I know of is bad relabel configs client side, resulting in series from different Prometheus instances to result in the same series within Cortex. In Prometheus' exemplar storage we rejects duplicate exemplars (description of what is considered a duplicate), and do the same in Cortex since we're just using Prometheus' exemplar storage currently.

Does this make sense, or am I missing something? cc @mdisibio

I think exemplars are handled the same way here since this is essentially a copy of util.MergeSampleSets

Thinking loudly:

util.MergeSampleSets is just used when gRPC streaming between queriers and ingesters is disabled (it's enabled by default).

When gRPC streaming is enabled and chunks transferring is disabled, we use mergeSamples() which doesn't compare samples value.

When both gRPC streaming and chunks transferring is enabled the ingester then distributorQuerier.streamingSelect() returns series.NewConcreteSeriesSet(). NewConcreteSeriesSet() iterates over series stored as chunkSeries so chunks are iterated by chunkSeries. chunkSeries.Iterator() uses the configured chunkIteratorFunc, so the behaviour depends on the configured chunk iteration function. The iteration function is returned by getChunksIteratorFunction(). Let's assume batch iteration is used (we do), then batch.NewChunkMergeIterator() is used. The deduplication is done by mergeStreams() which, in case of same timestamp, it just picks one of the two values without checking the actual value

So in all cases, I believe samples deduplication is equal to your mergeExemplarSets() function 👍

Given the time it took this analysis, what do you think adding a test case to mergeExemplarSets() to assert on the case "same timestamp but different value" so we write it into the ~~stone~~ tests? :)

Thanks again for the detailed explanation.

pkg/distributor/query.go

mdisibio · 2021-05-13T13:39:46Z

pkg/distributor/query.go

+			return err
+		}
+
+		replicationSet, err := d.GetIngestersForQuery(ctx, false, nil)


I think it would be good to clarify how all matchers are being sent to all ingesters for the query, even though on ingest they are sharded by series (i.e. it is expected that an ingester will satisfy only a subset of the matchers and the results will be merged). Maybe some comments, or a different name for the exemplarQuery parameter would be enough.

I agree a comment would be great.

Good point, I will add a comment.

For my own understanding, is this only true for shuffle sharding? even though on ingest they are sharded by series. I haven't followed shuffle sharding much. My assumption has always been that any data is replicated to all the ingesters for that tenant.

I'm also now thinking of whether we should just split this up more, getting the replica set for each set of matchers and running the exemplar queries with those, and then merging the results.

I'm also now thinking of whether we should just split this up more, getting the replica set for each set of matchers and running the exemplar queries with those, and then merging the results.

I don't think it's worth the effort. Let me explain why.

Regardless of shuffle-sharding (which is about tenants sharding, not series), series can be sharded across ingesters into two ways:

By metric name only

By all series

When sharding by all series you have to query all ingesters, while when sharding by metric name all series for a given metric name are sharded always to the same ingester (+ replicas). That's why we can restrict the ingesters to query from when "shard by metric name only" is used.

However, the current implementation of "query only a restricted set of ingesters when shard by metric name only is enabled" is also buggy and doesn't take in account reshardings (eg. scale up), so it's not even safe to use out of the box with the blocks storage.

Long story short, I think it's fine (and desired) to always query all ingesters for the exemplars. When shuffle-sharding is enabled, it will only query ingesters belonging to the tenant's shard (and that's OK).

@pracucci Thanks for the explanation! So in either scenario there's never 3 ingesters that have all of a given tenants data? A set of three would have a subset of their data based on the sharding type?

Not sure I understand your question. Could you elaborate more your question, please?

pracucci

Solid job @cstyan! Didn't find any issue. I left few minor comments I would be glad if you could take a look. Waiting for tests!

Could you also document the new API endpoints in docs/api/_index.md, please?

pracucci · 2021-05-13T14:13:22Z

pkg/api/handlers.go

@@ -254,6 +256,8 @@ func NewQuerierHandler(
 	router.Path(path.Join(legacyPrefix, "/api/v1/read")).Methods("POST").Handler(legacyPromRouter)
 	router.Path(path.Join(legacyPrefix, "/api/v1/query")).Methods("GET", "POST").Handler(legacyPromRouter)
 	router.Path(path.Join(legacyPrefix, "/api/v1/query_range")).Methods("GET", "POST").Handler(legacyPromRouter)
+	// unclear to me whether we need to register here


Let's keep it and then we will address this while router.Path() group in a separate PR, considering prometheus/prometheus#7125 has been merged.

we will address this while router.Path() group in a separate PR

Could you open an issue about it, please?

created: #4213

pkg/ingester/client/ingester.proto

pkg/ingester/ingester_v2.go

pkg/cortexpb/compat.go

pracucci · 2021-05-13T14:57:41Z

pkg/distributor/query.go

+			return err
+		}
+
+		replicationSet, err := d.GetIngestersForQuery(ctx, false, nil)


I agree a comment would be great.

pracucci · 2021-05-13T14:58:21Z

pkg/distributor/query.go

@@ -53,6 +54,32 @@ func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers .
 	return matrix, err
 }

+func (d *Distributor) QueryExemplars(ctx context.Context, from, to model.Time, matchers ...[]*labels.Matcher) (*ingester_client.QueryResponse, error) {
+	var result *ingester_client.QueryResponse
+	err := instrument.CollectedRequest(ctx, "Distributor.QueryExemplars", d.queryDuration, instrument.ErrorCode, func(ctx context.Context) error {


We're reusing d.queryDuration duration here. I think it's fine, but let's see how the other discussion about metrics evolve.

any thoughts here? we haven't discussed the rest of the metrics recently

I personally think it's fine starting tracking exemplar queries metrics along with "samples query" metrics to begin with (we're doing the same in ingester at query time). We may reconsider it as a future improvement, but I wouldn't block on this (I'm pretty sure we'll refine the exemplars implementation while learning more running it in production, as it happens for every feature we build).

As a followup to this it may be worth updating the help text for queryDuration. Currently it's:

Time spent executing expression queries.

It may be worth changing it to

Time spent executing expression and exemplar queries.

pkg/distributor/query.go

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pracucci · 2021-05-18T16:49:01Z

docs/api/_index.md

+
+Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).
+
+_For more information, please check out the Prometheus [range query](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries) documentation._


Let's link to the right one.

Suggested change

_For more information, please check out the Prometheus [range query](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries) documentation._

_For more information, please check out the Prometheus [querying exemplars](https://prometheus.io/docs/prometheus/latest/querying/api/#querying-exemplars) documentation._

pracucci · 2021-05-18T16:49:30Z

docs/api/_index.md

+GET,POST <legacy-http-prefix>/api/v1/query_exemplars
+```
+
+Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).


The query-frontend doesn't accelerate exemplars endpoint.

Suggested change

Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).

Prometheus-compatible exemplar query endpoint.

shouldn't we cache results?

The /api/v1/query_exemplars is not supported by query-frontend (at least not yet) so it's not getting accelerated. It's definitely something we can do, but it hasn't been done yet. Am I missing anything?

pracucci · 2021-05-18T16:53:01Z

pkg/ingester/ingester_v2.go

+		return nil, err
+	}
+
+	i.metrics.queries.Inc()


I don't know if Cortex has a 'metrics label changes are breaking changes' policy.

The breaking changes policy doesn't cover metrics so no policy blocker here. However, I would keep it simple in this PR and not add an extra label. It's something we can reconsider in the future IMO.

Reason why I would keep it simple is because if we add a label here we should probably do the same in other places (eg. query duration tracked by Distributor.QueryExemplars()).

pracucci

Thanks @cstyan ! The PR LGTM! I left few last minor comments. Could you also add at least a unit test to cover the querying part?

pracucci · 2021-05-18T16:56:57Z

pkg/distributor/query.go

+			result = append(result, b[j])
+			j++
+		} else {
+			result = append(result, a[i])


So my assumption is that while not every ingester in the replica set would have every exemplar, there shouldn't be any ingester that has an exemplar at a certain timestamp that doesn't exist in another ingester.

Think about this edge case:

Remote write exemplar for series X with TS=1 and V=1. Replication set is ingesters 1,2,3. This write succeed towards ingester 1 and 2, but not 3 (not ingested here).

Remote write exemplar for series X with TS=1 and V=2. Replication set is ingesters 1,2,3. This write succeed toward ingester 3 (was not ingested before) but fails on 1 and 2 (because of same timestamp but different value). Even if quorum has not been reached, the exemplar with V=2 is written anyway to ingester 3 (will not rollback from ingester 3 because writing to 1 and 2 has failed).

When you read back, you will have two exemplars with TS=1 and values 1 and 2.

The current implementation picks a random exemplar (between the two) if the timestamp is the same, without checking if value is equal as well. I personally think it's fine, but I wanted to outline the edge case above so you can take an informed decision.

pracucci · 2021-05-18T17:09:45Z

pkg/ingester/ingester_v2.go

+		return &client.ExemplarQueryResponse{}, nil
+	}
+
+	// Note that currently Prometheus' exemplar querier does nothing with a context that you pass it.


[nit] I would remove this comment just because these kind of comments age very badly :) If ctx will be used at some point by Prometheus, we'll very likely forget to remove this comment, but this comment can still deceive the reader/contributor.

pracucci · 2021-05-18T17:10:22Z

pkg/ingester/ingester_v2.go

+	// TODO should we update this series metric again?
+	// i.metrics.queriedSeries.Observe(float64(len(result.Timeseries)))


Thinking more about this, I believe we shouldn't. Because of this, I would remove this commented code.

pracucci · 2021-05-18T17:12:56Z

pkg/ingester/metrics.go

+		queriedExemplars: promauto.With(r).NewHistogram(prometheus.HistogramOpts{
+			Name: "cortex_ingester_queried_exemplars",
+			Help: "The total number of exemplars returned from queries.",
+			// TODO: think about buckets, guessing here.


To my understanding, max bucket is 1*(5^(5-1)) = 625. Maybe a bit low?

I've bumped the start to 10, this would give us buckets of 10, 50, 250, 1250, 6250. Let me know if you think this is reasonable. The likely hood of the exemplar in memory storage having very few exemplars per series is high. We could bump the # of buckets to 8 like the samples buckets, that would give us 10, 50, 250, 1250, 6250, 31250, 156250, 781250.

FWIW, out of the box, when you enable Prometheus exemplar storage, it has a max circular buffer size of 100k exemplars.

👍 I would remove the TODO.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan

Sorry for the delay, on-call has been noisy this week.

Updated the ingester push test (which also calls the query function for samples) to call the query exemplars function and confirm exemplars that are stored properly (we only had 1 test case for storing exemplars in that test), hadn't pushed that yet so here it is.

Addressed a few other small comments + replied to some, mostly metrics related.

cstyan · 2021-05-20T03:09:46Z

docs/api/_index.md

+GET,POST <legacy-http-prefix>/api/v1/query_exemplars
+```
+
+Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).


shouldn't we cache results?

cstyan · 2021-05-20T03:11:29Z

docs/api/_index.md

+
+Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).
+
+_For more information, please check out the Prometheus [range query](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries) documentation._


cstyan · 2021-05-20T03:13:07Z

pkg/distributor/query.go

@@ -53,6 +54,32 @@ func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers .
 	return matrix, err
 }

+func (d *Distributor) QueryExemplars(ctx context.Context, from, to model.Time, matchers ...[]*labels.Matcher) (*ingester_client.QueryResponse, error) {
+	var result *ingester_client.QueryResponse
+	err := instrument.CollectedRequest(ctx, "Distributor.QueryExemplars", d.queryDuration, instrument.ErrorCode, func(ctx context.Context) error {


any thoughts here? we haven't discussed the rest of the metrics recently

cstyan · 2021-05-20T03:40:26Z

pkg/distributor/query.go

+			result = append(result, b[j])
+			j++
+		} else {
+			result = append(result, a[i])


How does Cortex handle this situation for samples? I think exemplars are handled the same way here since this is essentially a copy of util.MergeSampleSets.

The only way this edge case could really happen that I know of is bad relabel configs client side, resulting in series from different Prometheus instances to result in the same series within Cortex. In Prometheus' exemplar storage we rejects duplicate exemplars (description of what is considered a duplicate), and do the same in Cortex since we're just using Prometheus' exemplar storage currently.

Does this make sense, or am I missing something? cc @mdisibio

cstyan · 2021-05-20T03:41:42Z

pkg/ingester/ingester_v2.go

+		return nil, err
+	}
+
+	i.metrics.queries.Inc()


would you prefer separate metrics altogether here?

cstyan · 2021-05-20T04:02:03Z

pkg/ingester/metrics.go

+		queriedExemplars: promauto.With(r).NewHistogram(prometheus.HistogramOpts{
+			Name: "cortex_ingester_queried_exemplars",
+			Help: "The total number of exemplars returned from queries.",
+			// TODO: think about buckets, guessing here.


I've bumped the start to 10, this would give us buckets of 10, 50, 250, 1250, 6250. Let me know if you think this is reasonable. The likely hood of the exemplar in memory storage having very few exemplars per series is high. We could bump the # of buckets to 8 like the samples buckets, that would give us 10, 50, 250, 1250, 6250, 31250, 156250, 781250.

FWIW, out of the box, when you enable Prometheus exemplar storage, it has a max circular buffer size of 100k exemplars.

pracucci

LGTM, thanks! I left few final nits, all very small things. Please also take a look at failed lint and unit tests.

[ENHANCEMENT] Blocks storage: support ingesting exemplars. Enabled by setting new CLI flag -blocks-storage.tsdb.max-exemplars=<n> or config option blocks_storage.tsdb.max_exemplars to positive value. Support ingesting exemplars into TSDB when blocks storage is enabled #4124

Could you update this CHANGELOG entry mentioning we support querying too + adding the PR number along side "#4124", please?

pracucci · 2021-05-20T11:04:41Z

docs/api/_index.md

+GET,POST <legacy-http-prefix>/api/v1/query_exemplars
+```
+
+Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).


The /api/v1/query_exemplars is not supported by query-frontend (at least not yet) so it's not getting accelerated. It's definitely something we can do, but it hasn't been done yet. Am I missing anything?

pracucci · 2021-05-20T11:07:15Z

pkg/distributor/query.go

@@ -53,6 +54,32 @@ func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers .
 	return matrix, err
 }

+func (d *Distributor) QueryExemplars(ctx context.Context, from, to model.Time, matchers ...[]*labels.Matcher) (*ingester_client.QueryResponse, error) {
+	var result *ingester_client.QueryResponse
+	err := instrument.CollectedRequest(ctx, "Distributor.QueryExemplars", d.queryDuration, instrument.ErrorCode, func(ctx context.Context) error {


I personally think it's fine starting tracking exemplar queries metrics along with "samples query" metrics to begin with (we're doing the same in ingester at query time). We may reconsider it as a future improvement, but I wouldn't block on this (I'm pretty sure we'll refine the exemplars implementation while learning more running it in production, as it happens for every feature we build).

pracucci · 2021-05-20T11:33:07Z

pkg/distributor/query.go

+			result = append(result, b[j])
+			j++
+		} else {
+			result = append(result, a[i])


I think exemplars are handled the same way here since this is essentially a copy of util.MergeSampleSets

Thinking loudly:

util.MergeSampleSets is just used when gRPC streaming between queriers and ingesters is disabled (it's enabled by default).

When gRPC streaming is enabled and chunks transferring is disabled, we use mergeSamples() which doesn't compare samples value.

When both gRPC streaming and chunks transferring is enabled the ingester then distributorQuerier.streamingSelect() returns series.NewConcreteSeriesSet(). NewConcreteSeriesSet() iterates over series stored as chunkSeries so chunks are iterated by chunkSeries. chunkSeries.Iterator() uses the configured chunkIteratorFunc, so the behaviour depends on the configured chunk iteration function. The iteration function is returned by getChunksIteratorFunction(). Let's assume batch iteration is used (we do), then batch.NewChunkMergeIterator() is used. The deduplication is done by mergeStreams() which, in case of same timestamp, it just picks one of the two values without checking the actual value

So in all cases, I believe samples deduplication is equal to your mergeExemplarSets() function 👍

Given the time it took this analysis, what do you think adding a test case to mergeExemplarSets() to assert on the case "same timestamp but different value" so we write it into the ~~stone~~ tests? :)

pracucci · 2021-05-20T12:05:53Z

pkg/ingester/ingester_v2.go

+		return nil, err
+	}
+
+	i.metrics.queries.Inc()


I would work on it in a follow up PR. I generally suggest to work iteratively. It's fine merging this PR as is and discuss the metrics separation in a follow up PR.

pracucci · 2021-05-20T12:06:27Z

pkg/ingester/metrics.go

+		queriedExemplars: promauto.With(r).NewHistogram(prometheus.HistogramOpts{
+			Name: "cortex_ingester_queried_exemplars",
+			Help: "The total number of exemplars returned from queries.",
+			// TODO: think about buckets, guessing here.


👍 I would remove the TODO.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pracucci

LGTM! Thanks!

cstyan · 2021-05-27T05:39:45Z

I found some issues while deploying/testing this that I will fix later this week.

Dismissing because Callum mentioned they found some issues while testing. Waiting to learn more about it.

jtlisi

LGTM

jtlisi · 2021-05-28T13:31:04Z

pkg/distributor/query.go

@@ -53,6 +54,32 @@ func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers .
 	return matrix, err
 }

+func (d *Distributor) QueryExemplars(ctx context.Context, from, to model.Time, matchers ...[]*labels.Matcher) (*ingester_client.QueryResponse, error) {
+	var result *ingester_client.QueryResponse
+	err := instrument.CollectedRequest(ctx, "Distributor.QueryExemplars", d.queryDuration, instrument.ErrorCode, func(ctx context.Context) error {


As a followup to this it may be worth updating the help text for queryDuration. Currently it's:

Time spent executing expression queries.

It may be worth changing it to

Time spent executing expression and exemplar queries.

jtlisi · 2021-05-28T13:37:14Z

pkg/distributor/query.go

+			}
+			// Merge in any missing values from another ingesters exemplars for this series.
+			e.Exemplars = mergeExemplarSets(e.Exemplars, ts.Exemplars)


suggestion: I may be missing something, but if I'm not mistaken, there is no need to call this function if there isn't an existing result, correct?

Suggested change

}

// Merge in any missing values from another ingesters exemplars for this series.

e.Exemplars = mergeExemplarSets(e.Exemplars, ts.Exemplars)

} else {

// Merge in any missing values from another ingesters exemplars for this series.

e.Exemplars = mergeExemplarSets(e.Exemplars, ts.Exemplars)

}

Yeah probably not, I don't think it's functionally any different. I guess there's the potential for slightly more memory being allocated than is really necessary without the else. Is this what you were thinking?

pkg/ingester/metrics.go

jtlisi · 2021-05-28T13:44:24Z

pkg/distributor/query_test.go

@@ -106,3 +109,52 @@ func TestMergeSamplesIntoFirstNilB(t *testing.T) {

 	require.Equal(t, b, a)
 }
+
+func TestMergeExemplarSets(t *testing.T) {


Clean test! 👍

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pracucci · 2021-06-01T16:20:59Z

pkg/querier/distributor_queryable.go

+		// TODO: (callum) track down why we see empty exemplars slices here
+		// but not in the distributor/ingester code.


Can you elaborate on this? This looks something worth to investigate before merging this PR.

When I first deployed this branch to one of our Cortex clusters I noticed intermittent query failures in Grafana itself, and looking at the query response I saw a lot of results (they always seemed to be at the beginning of the overall []exemplar.QueryResult slice) that had empty series labels and 0 len exemplar slices. The if condition right below was my quick fix.

Yesterday I deployed these changes with additional debug logging, and this block was the only place I saw empty results. Which doesn't really make sense to me. This TODO is a note to me to look into this more. If there's a way to mock a query result from an Ingester replica set then I could try and reproduce in a test, didn't see anything obvious.

Fixed the issue in the latest commit(s).

Signed-off-by: Callum Styan <callumstyan@gmail.com>

to cortex. Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan · 2021-06-09T19:21:12Z

Let me know if I should update all the example docker compose setups the same as the one for local s3 as I've done in the latest commit.

pracucci

LGTM! Thanks for addressing all feedback 🙏

Let me know if I should update all the example docker compose setups the same as the one for local s3 as I've done in the latest commit.

Not strictly required (still a nice to have). Definitely not a blocker for this PR. We can add it anytime, whenever required.

Initial commit; querying of exemplars.

cc51f3f

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pull-request-size bot added the size/L label May 13, 2021

mdisibio reviewed May 13, 2021

View reviewed changes

pkg/distributor/query.go Show resolved Hide resolved

mdisibio reviewed May 13, 2021

View reviewed changes

pracucci reviewed May 13, 2021

View reviewed changes

Address review comments

6254ef2

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pracucci reviewed May 18, 2021

View reviewed changes

address review feedback

9e440b0

Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan commented May 20, 2021

View reviewed changes

pracucci approved these changes May 20, 2021

View reviewed changes

address final review feedback

f6abe52

Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan changed the title ~~WIP: exemplar querying~~ exemplar querying May 24, 2021

pracucci previously approved these changes May 25, 2021

View reviewed changes

jtlisi approved these changes May 28, 2021

View reviewed changes

cstyan added 2 commits May 31, 2021 20:41

Fixes from testing in an actual cluster.

3588940

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Add comments suggested by Jacob.

aa26af2

Signed-off-by: Callum Styan <callumstyan@gmail.com>

pracucci reviewed Jun 1, 2021

View reviewed changes

cstyan added 2 commits June 8, 2021 17:39

Fix empty result issue with exemplar queries.

ed3b913

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Update local docker-compose s3 to remote write exemplars from prometheus

9fd50af

to cortex. Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan requested a review from pracucci June 15, 2021 15:28

pracucci approved these changes Jun 15, 2021

View reviewed changes

pracucci merged commit 82b32ec into cortexproject:master Jun 15, 2021


		Prometheus-compatible exemplar query endpoint. When the request is sent through the query-frontend, the query will be accelerated by query-frontend (results caching and execution parallelisation).

		_For more information, please check out the Prometheus [range query](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries) documentation._

		// TODO should we update this series metric again?
		// i.metrics.queriedSeries.Observe(float64(len(result.Timeseries)))

		// TODO: (callum) track down why we see empty exemplars slices here
		// but not in the distributor/ingester code.

exemplar querying #4181

exemplar querying #4181

Uh oh!

Conversation

cstyan commented May 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci May 18, 2021 •

edited

Loading

pracucci left a comment •

edited

Loading