Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

bryanhuhta
Copy link
Contributor

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:

'1218':
  tenant_settings_overrides:
    theme: dark

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 of theme via the /Set API. If this is attempting, the caller will get the following error:

{
  "code": "internal",
  "message": "failed to update theme: setting is readonly"
}

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 the readonly field marked as true.

{
  "settings": [
    {
      "name": "theme",
      "value": "dark",
      "readonly": true
    }
  ]
}

This will be helpful for the UI to render these settings differently to indicate they are readonly.

@bryanhuhta bryanhuhta self-assigned this Nov 12, 2024
@bryanhuhta bryanhuhta requested review from a team as code owners November 12, 2024 23:56
@bryanhuhta
Copy link
Contributor Author

I'm looking now at how the UI leverages tenant settings and it looks like everything is bucketed under a single entry pluginSettings.

{
  "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

  • Introspect the setting key value to see if we can read the json and further block keys
  • Refactor the UI to use a single setting key/value pair per setting

Copy link
Contributor

@knylander-grafana knylander-grafana left a 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!

Comment on lines +94 to +96
// Tenant settings.
TenantSettingsOverrides map[string]string `yaml:"tenant_settings_overrides" json:"tenant_settings_overrides"`

Copy link
Collaborator

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

@grafakus
Copy link
Collaborator

grafakus commented Nov 13, 2024

I'm looking now at how the UI leverages tenant settings and it looks like everything is bucketed under a single entry pluginSettings.
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

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?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants