-
Notifications
You must be signed in to change notification settings - Fork 526
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
Added cluster seed support in preparation of anonymous usage reporter #2643
Conversation
var stores BlocksStoreSet | ||
var ( | ||
stores BlocksStoreSet | ||
bucketClient objstore.Bucket |
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.
Note to reviewers: define bucketClient objstore.Bucket
because now bucket.NewClient()
returns a objstore.InstrumentedBucket
.
@@ -119,7 +123,7 @@ type Config struct { | |||
|
|||
// Not used internally, meant to allow callers to wrap Buckets | |||
// created using this config | |||
Middlewares []func(objstore.Bucket) (objstore.Bucket, error) `yaml:"-"` | |||
Middlewares []func(objstore.InstrumentedBucket) (objstore.InstrumentedBucket, error) `yaml:"-"` |
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.
Note to reviewers: I will take care to re-vendor Mimir in GEM and fix Middlewares there.
|
||
level.Info(r.logger).Log("msg", "usage reporter initialized", "cluster_id", seed.UID) | ||
|
||
// TODO Periodically send usage report. |
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.
Note to reviewers: this will be done in the next PR.
pkg/mimir/mimir.go
Outdated
@@ -115,6 +116,7 @@ type Config struct { | |||
RuntimeConfig runtimeconfig.Config `yaml:"runtime_config"` | |||
MemberlistKV memberlist.KVConfig `yaml:"memberlist"` | |||
QueryScheduler scheduler.Config `yaml:"query_scheduler"` | |||
UsageReport usagestats.Config `yaml:"usage_reporter"` |
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.
On this line there's:
- UsageReport
- usagestats
- usage_reporter
Can we have a single name for everything?
My 5 cents: I'd call everything UsageStats
/usagestats
/usage_stats
, and have a -usage-stats.enabled
CLI flag.
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.
Makes a lot of sense. Was inherited by Loki (where there's no consistency), then I mangled it without adding consistency tho 🤦
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, please consider unifying the UsageReport/usagestags/usage_reporter naming into at most 2 names (if not 1).
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
19e02ea
to
07768cc
Compare
} | ||
|
||
func (m *DelayedBucketClient) Get(ctx context.Context, name string) (io.ReadCloser, error) { | ||
m.delay() |
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.
The delay should be deferred too, which IMO is more important, as this way we simulate the delay of observation of the real world (API told us that there was no item, but while we got the response, one could be created).
I think the best option is to do both:
m.delay()
defer m.delay()
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.
Still LGTM :)
Please consider deferring the delay too.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
This PR is a first step to build the anonymous usage reporter as described in #1815.
In this PR I'm introducing the support for the cluster seed, stored in a dedicated prefix called
__mimir_cluster/
. The reason why I propose to use a dedicated prefix instead of storing the seed file in the root of the bucket is to have a prefix reserved for Mimir data in object storage. This also means that, starting from now,__mimir_cluster
is no more a valid tenant ID.Which issue(s) this PR fixes or relates to
Part of #1815
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]