-
Notifications
You must be signed in to change notification settings - Fork 7
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
receiver: Add hot reload for relabel configs #124
receiver: Add hot reload for relabel configs #124
Conversation
@@ -143,14 +137,6 @@ func NewLimiterWithOptions( | |||
Help: "How many times the limit configuration failed to reload.", | |||
}, | |||
) | |||
limiter.configReloadFailedCounter = promauto.With(limiter.registerer).NewCounter( |
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.
Removed because this is present twice here (see line 138 just above)
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.
left a few comments, please test the branch following this https://databricks.atlassian.net/wiki/spaces/UN/pages/3754591684/Pantheon+knowledge+base#Testing-Guide
TSDBStats: dbs, | ||
Limiter: limiter, | ||
|
||
Writer: writer, |
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: no change happens here right?
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.
yes only whitespace changes by gofmt that does some kind of alignment of the fields.
cmd/thanos/receive.go
Outdated
@@ -1073,6 +1086,8 @@ func (rc *receiveConfig) registerFlag(cmd extkingpin.FlagClause) { | |||
rc.maxBackoff = extkingpin.ModelDuration(cmd.Flag("receive-forward-max-backoff", "Maximum backoff for each forward fan-out request").Default("5s").Hidden()) | |||
|
|||
rc.relabelConfigPath = extflag.RegisterPathOrContent(cmd, "receive.relabel-config", "YAML file that contains relabeling configuration.", extflag.WithEnvSubstitution()) | |||
cmd.Flag("receive.relabel-config-reload-timer", "Minimum amount of time to pass for the relabel configuration to be reloaded. Helps to avoid excessive reloads."). | |||
Default("1s").Hidden().DurationVar(&rc.relabelConfigReloadTimer) |
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.
default 1s could be excessive, let's do 5m
for default, i assume the relabel config should not change that frequently.
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.
+1
Can we have a default value that disables this feature?
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.
Ok. I went by the same value as for the limiters config file that is reloaded every 1s by default.
Line 1159 in 779d964
cmd.Flag("receive.limits-config-reload-timer", "Minimum amount of time to pass for the limit configuration to be reloaded. Helps to avoid excessive reloads."). |
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.
Changed default to 0 with the semantics of disabling the feature.
testutil.Ok(t, err) | ||
|
||
// Rewrite the relabels file in temp with an invalid file, should not update the config. | ||
testutil.Ok(t, relabelFile.Rewrite(invalidRelabels)) |
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.
how do we know the invalid relabels are not picked up
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.
There is an assert below on line 56 that the value is the same as origRelabelsConfig.
cmd/thanos/receive.go
Outdated
@@ -1073,6 +1086,8 @@ func (rc *receiveConfig) registerFlag(cmd extkingpin.FlagClause) { | |||
rc.maxBackoff = extkingpin.ModelDuration(cmd.Flag("receive-forward-max-backoff", "Maximum backoff for each forward fan-out request").Default("5s").Hidden()) | |||
|
|||
rc.relabelConfigPath = extflag.RegisterPathOrContent(cmd, "receive.relabel-config", "YAML file that contains relabeling configuration.", extflag.WithEnvSubstitution()) | |||
cmd.Flag("receive.relabel-config-reload-timer", "Minimum amount of time to pass for the relabel configuration to be reloaded. Helps to avoid excessive reloads."). | |||
Default("1s").Hidden().DurationVar(&rc.relabelConfigReloadTimer) |
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.
+1
Can we have a default value that disables this feature?
0741201
to
7f4a0b9
Compare
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, thank you for working on this one, please rebase your pr, I've fixed some of the failing tests in #128
7f4a0b9
to
e6fcd04
Compare
Changes
Verification