-
Notifications
You must be signed in to change notification settings - Fork 610
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
feat: Tenant settings overrides #3676
base: main
Are you sure you want to change the base?
Conversation
I'm looking now at how the UI leverages tenant settings and it looks like everything is bucketed under a single entry {
"setting": {
"name": "pluginSettings",
"value": "{\"collapsedFlamegraphs\":false,\"maxNodes\":16384,\"enableFlameGraphDotComExport\":true,\"enableFunctionDetails\":true}"
}
} Undoubtably this makes it very convenient to read/set/update settings from the UI. However, this does make the changes in this PR ineffective at marking a single setting as read-only. We will either need to
|
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.
Thank you for updating the docs!
// Tenant settings. | ||
TenantSettingsOverrides map[string]string `yaml:"tenant_settings_overrides" json:"tenant_settings_overrides"` | ||
|
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 guess we should also register this as a CLI flag for consistency with other limits. RelabelRules
is a good example of how a structured configuration may be handled. Also, a list representation of settings will probably be easier to manage
Actually no, because the UI has to parse, extract and validate the fields it's interested in. IIRC the API was designed for maximum flexibility, which was a good idea back in the days. The change suggested in this PR would be a breaking change ; my concern is that the UI will have to support backwards compatibility for an undefined period of time... As discussed recently during our weekly, could we (for the time being and for this specific case) find a backward-compatible alternative? Something like: {
modifiedAt: number;
name: "pluginSettings";
value: string;
permissions: Permission[];
}
Permission = { name: string, value: 'read-only' } Sorry if it's a simplistic solution to a problem that may be more complex, I'm unaware of all the subtleties and consequences involved for the backend (e.g. do we want to expose the permissions in the API response?). |
closes https://github.com/grafana/pyroscope-squad/issues/276
This modifies the tenant-settings service to allow per-tenant overrides of settings values. To configure an override, add a tenant override like so:
This will make sure the
theme
setting always returns "dark" regardless of the underlying value stored in the bucket. Additionally, this PR makes it impossible to change the value oftheme
via the/Set
API. If this is attempting, the caller will get the following error:The API response has been modified to include an extra field
readonly
. If an override has been placed on a setting name, it will have thereadonly
field marked astrue
.This will be helpful for the UI to render these settings differently to indicate they are readonly.