Use kv package from github.com/grafana/dskit #4436
Conversation
aknuds1
force-pushed
the
chore/use-dskit-kv
branch
from
c53b002
to
8e64f12
Compare
There was a problem hiding this comment.
We should upgrade Thanos first in a separate PR and double check all other dependencies which are getting updated here.
For example, I think the grpc 1.39.0 upgrade is not safe here, because of the change introduced by grpc/grpc-go#4416.
@pracucci thanks for the feedback, I can make a PR to upgrade in another PR then. |
aknuds1
force-pushed
the
chore/use-dskit-kv
branch
6 times, most recently
from
5bb503d
to
13a6d26
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
aknuds1
force-pushed
the
chore/use-dskit-kv
branch
from
13a6d26
to
6d04cae
Compare
|
The PR should be ready for review now. |
| # IP, hostname or an entry specified in the DNS Service Discovery format (see | ||
| # https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery | ||
| # for more details). | ||
| # IP, hostname or an entry specified in the DNS Service Discovery format. |
There was a problem hiding this comment.
I guess this changed because we didn't want a reference from dskit to Cortex, but should we not still have a reference to the details?
There was a problem hiding this comment.
@bboreham what would be a good way to solve this? I personally don't know.
There was a problem hiding this comment.
Could add the text from https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery to the dskit repo, then point to it there?
There was a problem hiding this comment.
@bboreham good suggestion, thanks.
|
|
||
| // Make sure that no DNS failures occurred. | ||
| // No actual DNS lookups are necessarily performed, so we can't really assert on that. | ||
| mlMatcher := labels.MustNewMatcher(labels.MatchEqual, "name", "memberlist") | ||
| require.NoError(t, cortex1.WaitSumMetricsWithOptions(e2e.Equals(0), []string{"cortex_dns_failures_total"}, e2e.WithLabelMatchers(mlMatcher))) | ||
| require.NoError(t, cortex2.WaitSumMetricsWithOptions(e2e.Equals(0), []string{"cortex_dns_failures_total"}, e2e.WithLabelMatchers(mlMatcher))) |
There was a problem hiding this comment.
Can you explain why this is relevant to the PR?
There was a problem hiding this comment.
@bboreham it's in order to catch a potential regression where the metric will become dns_failures_total unless the Cortex code passes a registerer with prefix cortex_ to dskit/kv. Does that clarify the connection to the PR?
The test was added due to initially having this regression while working on the PR.
There was a problem hiding this comment.
That would be a good thing to explain in the commit message.
There was a problem hiding this comment.
@bboreham OK will keep it in mind for the future.
)" This reverts commit 32b1b40. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
)" This reverts commit 32b1b40. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
)" (cortexproject#4611) This reverts commit 32b1b40. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
What this PR does:
In this PR I propose to refactor to use dskit's
kvpackage instead ofpkg/ring/kv.Which issue(s) this PR fixes:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]