grafana / pyroscope

Continuous Profiling Platform. Debug performance issues down to a single line of code
https://grafana.com/oss/pyroscope/
GNU Affero General Public License v3.0
10.14k stars 614 forks source link

feat: Tenant settings overrides #3676

Closed bryanhuhta closed 1 week ago

bryanhuhta commented 2 weeks ago

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 commented 2 weeks ago

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

grafakus commented 2 weeks ago

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

bryanhuhta commented 1 week ago

Closing this PR as the team works to land on a better implementation to the problem of locking down certain tenant settings.