-
Notifications
You must be signed in to change notification settings - Fork 524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make ephemeral storage queryable #3961
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving help text.
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
053186d
to
e76664b
Compare
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
…turn empty results. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
pkg/ingester/ingester.go
Outdated
@@ -107,6 +107,18 @@ const ( | |||
|
|||
// Prefix used in Prometheus registry for ephemeral storage. | |||
ephemeralPrometheusMetricsPrefix = "ephemeral_" | |||
|
|||
// Label name used to select queried storage type. | |||
StorageLabelName = "__storage__" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use something less generic, eg. __mimir_storage__
here to avoid possible conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is only an internal label which most users will never get to see, I think making it specific would be the better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate topic: should we prevent the successful ingestion of this label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also 👍 to use __mimir_storage__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we prevent the successful ingestion of this label?
I think the risk is tiny, and it would complicate our code. I'd leave it as-is (no extra check on push).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the logic and LGTM! I haven't looked at the tests (I rely on Mauro's review ;))
pkg/ingester/ingester.go
Outdated
@@ -107,6 +107,18 @@ const ( | |||
|
|||
// Prefix used in Prometheus registry for ephemeral storage. | |||
ephemeralPrometheusMetricsPrefix = "ephemeral_" | |||
|
|||
// Label name used to select queried storage type. | |||
StorageLabelName = "__storage__" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also 👍 to use __mimir_storage__
.
pkg/ingester/ingester.go
Outdated
storageValue, matchers, err := removeStorageMatcher(matchers) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
ephemeral := false | ||
if storageValue == "" || storageValue == PersistentStorageLabelValue { | ||
// storageValue is logged later, so set it to proper value. | ||
storageValue = PersistentStorageLabelValue | ||
} else if storageValue == EphemeralStorageLabelValue { | ||
ephemeral = true | ||
} else { | ||
return fmt.Errorf(errInvalidStorageLabelValue, storageValue) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] To keep this function cleaner, you may consider moving all of this to a dedicated function detectStorageMatcher()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved check for accepted label values to removeStorageMatcherAndGetStorageType
(previously removeStorageMatcher
) in 2c15e15
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What this PR does
This PR modifies ingester to allow querying of ephemeral storage. This is done by special label
__storage__
that ingester recognizes. It allows two values:ephemeral
orpersistent
. If label is not in the matchers, ingester uses it aspersistent
. Any other values are rejected with error.This PR also introduces new metrics:
cortex_ingester_queries_ephemeral_total
(based on existingcortex_ingester_queries_total
)cortex_ingester_queried_ephemeral_samples
(based on existingcortex_ingester_queried_samples
)cortex_ingester_queried_ephemeral_series
(based on existingcortex_ingester_queried_series
)Which issue(s) this PR fixes or relates to
This is part of #3884. Changelog entry will be added after feature is complete.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]