LabelNames API with matchers #3

colega · 2021-07-14T17:31:16Z

What this PR does: Implements support for matchers in LabelQuerier.LabelNames() as in prometheus/prometheus#9083

Which issue(s) this PR fixes:
Fixes cortexproject/cortex#3658

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
LabelNames API with matchers prometheus/prometheus#9083 is merged and first commit in this pull request is rewritten to use prometheus/main.

pracucci

Could you open a preliminary PR to upgrade Prometheus, Alertmanager and Prometheus common (+ their deps)? It's fine if doesn't include your change yet, but will reduce the review surface, splitting the review effort into smaller subpieces.

colega · 2021-07-15T09:04:25Z

@pracucci sure, I was just exploring the changes needed here to adopt my change.

I actually find other changes (in alert manager and tsdb constructor) more ~~complicated~~ tedious to implement.

pkg/querier/blocks_store_queryable.go

go.mod

colega · 2021-07-16T14:09:48Z

@pracucci the only thing pending here is prometheus/prometheus#9083 to be merged, however I don't expect any significant changes there so I would appreciate a first review on this to see if there's something important I'm missing.

No rush, obviously, because it's blocked until next week anyway.

colega · 2021-07-20T13:02:52Z

prometheus/prometheus#9083 is merged, once we merge #16 I'll rebase this.

pracucci · 2021-07-20T14:04:35Z

prometheus/prometheus#9083 is merged, once we merge #16 I'll rebase this.

Merged! Could you rebase, please?

Updated Prometheus to latest commit that includes changes in the LabelQuerier.LabelNames method signature: prometheus/prometheus#9083 Signed-off-by: Oleg Zaytsev <[email protected]>

Signed-off-by: Oleg Zaytsev <[email protected]>

And fix missing matchers in the call to Distributor Signed-off-by: Oleg Zaytsev <[email protected]>

Signed-off-by: Oleg Zaytsev <[email protected]>

colega · 2021-07-20T15:04:48Z

And rebased again since there were new conflicting changes in main

pracucci

Impeccable job! It's uncommon seeing such good PRs. Well done on tests too. I left few comments I would be glad if you could address: a part from this, LGTM!

CHANGELOG.md

pkg/distributor/distributor_test.go

pracucci · 2021-07-21T15:38:53Z

pkg/distributor/distributor_test.go

+			expectedResult:    []string{labels.MetricName, "status"},
+			expectedIngesters: numIngesters,
+		},
+		"should return all matching metrics even if their FastFingerprint collide": {


This test case comes from TestDistributor_MetricsForLabelMatchers but actually doesn't test that a fingerprint collision doesn't hide away label names (cause the two series have the same label names). Not sure it's worth spending time to write a better test case for the fingerprint collision, we could probably just get rid of the fingerprint collision test case of LabelNames().

Removed the testcase until I find two labels with different names but same fingerprint

pkg/querier/querier.go

pkg/querier/distributor_queryable.go

pkg/querier/blocks_store_queryable.go

Signed-off-by: Oleg Zaytsev <[email protected]>

If multiple matchers didn't work, we wouldn't realize as the result set was the same as for a single matcher. Now the multiple matcher doesn't match the series with status=500 which has a reason=broken label Signed-off-by: Oleg Zaytsev <[email protected]>

Signed-off-by: Oleg Zaytsev <[email protected]>

All the StoreGateways (actually, one) are already supporting matchers in the call so we don't need to use a feature flag on this. Signed-off-by: Oleg Zaytsev <[email protected]>

Signed-off-by: Oleg Zaytsev <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

pracucci

Still LGTM with a couple of final nits.

CHANGELOG.md

pkg/querier/blocks_store_queryable.go

Signed-off-by: Oleg Zaytsev <[email protected]>

At least until we find a collision between two different label names. Signed-off-by: Oleg Zaytsev <[email protected]>

pracucci

LGTM, thanks! 🚀

pstibrany

Great work!

I have a question about whether implementation in tenant-federation code is correct though. Please take a look.

pkg/querier/tenantfederation/merge_queryable.go

Signed-off-by: Oleg Zaytsev <[email protected]>

Since there are three options, IMO, a switch is clearer than if/continue/if/elseif structure. Signed-off-by: Oleg Zaytsev <[email protected]>

pstibrany

LGTM, thanks for great work and addressing my feedback.

bboreham · 2021-08-12T17:09:52Z

What this PR does: Implements support for matchers in LabelQuerier.LabelValues

That's a typo, right? Should say LabelNames ?

colega · 2021-08-18T12:48:07Z

Oh yes, @bboreham definitely. Will edit the description.

Signed-off-by: Peter Štibraný <[email protected]> Former-commit-id: 06bbda1

Signed-off-by: Peter Štibraný <[email protected]>

# This is the 1st commit message: Add MaxTotalQueryLength and fill it from MaxQueryLength if 0 Add the ability to define a higher total query length limit on the frontend , compared to the current shared limit between querier and query-frontend. Ref: grafana/mimir-squad#889 Signed-off-by: György Krajcsovits <[email protected]> # Conflicts: # CHANGELOG.md # This is the commit message #2: Update runbook # This is the commit message #3: Update pkg/util/validation/limits.go Co-authored-by: Nick Pillitteri <[email protected]> # This is the commit message #4: Rename store.max-total-query-length to query-frontend... Signed-off-by: György Krajcsovits <[email protected]>

check presence of recording rule data for query window

pracucci reviewed Jul 14, 2021

View reviewed changes

colega changed the base branch from main to colega/upgrade-prometheus-2021-07 July 16, 2021 07:29

colega force-pushed the label-names-api-with-matchers branch from 7e4015b to fc1b460 Compare July 16, 2021 07:34

colega commented Jul 16, 2021

View reviewed changes

pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved

colega commented Jul 16, 2021

View reviewed changes

pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved

Base automatically changed from colega/upgrade-prometheus-2021-07 to main July 16, 2021 13:05

colega force-pushed the label-names-api-with-matchers branch 2 times, most recently from 6b2ec46 to f056488 Compare July 16, 2021 13:17

colega commented Jul 16, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

colega changed the title ~~WIP: Label names api with matchers~~ LabelNames API with matchers Jul 16, 2021

colega force-pushed the label-names-api-with-matchers branch from ae707d5 to a149798 Compare July 20, 2021 14:22

colega marked this pull request as ready for review July 20, 2021 14:23

colega added 14 commits July 20, 2021 17:02

Upgrade prometheus to b1ed4a0a663d0c62526312311c7529471abbc565

86e6e55

Updated Prometheus to latest commit that includes changes in the LabelQuerier.LabelNames method signature: prometheus/prometheus#9083 Signed-off-by: Oleg Zaytsev <[email protected]>

Add matchers to all LabelNames implementations

8b2a36b

Signed-off-by: Oleg Zaytsev <[email protected]>

Pass matchers from distributor to ingester's block tsdb

5329ae5

Signed-off-by: Oleg Zaytsev <[email protected]>

Ingester.LabelNames(w/matchers) for chunks too

8793512

Signed-off-by: Oleg Zaytsev <[email protected]>

Use the old distributor query path when cfg is disabled

9e32615

Signed-off-by: Oleg Zaytsev <[email protected]>

Sort label names in old implementations

99aa870

Signed-off-by: Oleg Zaytsev <[email protected]>

BlockStoreQueryable querying both paths

a31b302

Signed-off-by: Oleg Zaytsev <[email protected]>

Fix pkg/querier/querier_test.go

2342f8d

Signed-off-by: Oleg Zaytsev <[email protected]>

Use the provided ctx

c74a9b1

Signed-off-by: Oleg Zaytsev <[email protected]>

Make doc

d476d30

Test distributorQueryable.LabelNames

f241e5e

And fix missing matchers in the call to Distributor Signed-off-by: Oleg Zaytsev <[email protected]>

Test Distributor.LabelNames

bd8492b

Signed-off-by: Oleg Zaytsev <[email protected]>

Test Ingester.LabelNames with matchers

893228a

Signed-off-by: Oleg Zaytsev <[email protected]>

Add matchers to querier test

5a7ea80

Signed-off-by: Oleg Zaytsev <[email protected]>

Add CHANGELOG.md entry

f9f5518

Signed-off-by: Oleg Zaytsev <[email protected]>

colega force-pushed the label-names-api-with-matchers branch from a149798 to f9f5518 Compare July 20, 2021 15:04

pracucci approved these changes Jul 22, 2021

View reviewed changes

colega and others added 6 commits July 22, 2021 10:19

Move changes to mimir/unreleased changelog section

ce8a310

Signed-off-by: Oleg Zaytsev <[email protected]>

Update docs, add enabled suffix to the config.

01e894d

Signed-off-by: Oleg Zaytsev <[email protected]>

Remove fixme comment and feature flag from blocks_store_queryable

6b451eb

All the StoreGateways (actually, one) are already supporting matchers in the call so we don't need to use a feature flag on this. Signed-off-by: Oleg Zaytsev <[email protected]>

Update span name

79ab2db

Signed-off-by: Oleg Zaytsev <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

Merge branch 'main' into label-names-api-with-matchers

28aa6a8

pracucci approved these changes Jul 22, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved

colega added 3 commits July 22, 2021 13:00

Remove feature flag from blocks_store_queryable

0e3681d

Signed-off-by: Oleg Zaytsev <[email protected]>

Fixup changelog entry

2809837

Signed-off-by: Oleg Zaytsev <[email protected]>

Remove fingerprint collision testcase

70d23fd

At least until we find a collision between two different label names. Signed-off-by: Oleg Zaytsev <[email protected]>

pracucci approved these changes Jul 22, 2021

View reviewed changes

pstibrany reviewed Jul 22, 2021

View reviewed changes

pkg/querier/tenantfederation/merge_queryable.go Outdated Show resolved Hide resolved

colega added 2 commits July 23, 2021 10:43

Handle matchers in tenantfederation/mergeQuerier.LabelNames

20597a8

Signed-off-by: Oleg Zaytsev <[email protected]>

Opinionated refactor of filterValuesByMatchers

8f51e15

Since there are three options, IMO, a switch is clearer than if/continue/if/elseif structure. Signed-off-by: Oleg Zaytsev <[email protected]>

colega force-pushed the label-names-api-with-matchers branch from 0ee4c74 to 8f51e15 Compare July 23, 2021 08:44

pstibrany approved these changes Jul 23, 2021

View reviewed changes

pstibrany merged commit 17e3423 into main Jul 23, 2021

pstibrany deleted the label-names-api-with-matchers branch July 23, 2021 12:00

simonswine pushed a commit that referenced this pull request Oct 14, 2021

Prepare release 1.7.1. (#3)

d321971

Signed-off-by: Peter Štibraný <[email protected]> Former-commit-id: 06bbda1

pstibrany added a commit that referenced this pull request Feb 28, 2022

Make linter happy #3

88ffd20

Signed-off-by: Peter Štibraný <[email protected]>

replay pushed a commit that referenced this pull request Aug 8, 2024

Merge pull request #3 from replay/labels-start-end

c354336

check presence of recording rule data for query window

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LabelNames API with matchers #3

LabelNames API with matchers #3

colega commented Jul 14, 2021 •

edited

Loading

pracucci left a comment

colega commented Jul 15, 2021

colega commented Jul 16, 2021

colega commented Jul 20, 2021

pracucci commented Jul 20, 2021

colega commented Jul 20, 2021

pracucci left a comment

pracucci Jul 21, 2021

colega Jul 22, 2021

pracucci left a comment

pracucci left a comment

pstibrany left a comment

pstibrany left a comment

bboreham commented Aug 12, 2021

colega commented Aug 18, 2021

LabelNames API with matchers #3

LabelNames API with matchers #3

Conversation

colega commented Jul 14, 2021 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

colega commented Jul 15, 2021

colega commented Jul 16, 2021

colega commented Jul 20, 2021

pracucci commented Jul 20, 2021

colega commented Jul 20, 2021

pracucci left a comment

Choose a reason for hiding this comment

pracucci Jul 21, 2021

Choose a reason for hiding this comment

colega Jul 22, 2021

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

bboreham commented Aug 12, 2021

colega commented Aug 18, 2021

colega commented Jul 14, 2021 •

edited

Loading