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.

Sign up for GitHub

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

Jump to bottom

Add HTML pages to view tenants with overrides #3332

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Add HTML pages to view tenants with overrides #3332

merged 8 commits into from
Feb 1, 2024

Conversation

kvrhdn
Copy link
Member

@kvrhdn kvrhdn commented Jan 25, 2024 •
edited
Loading

What this PR does:
Changes /status/overrides and /status/overrides/{tenant} to HTML page which are nicer and easier to use.

Examples:

/status/overrides with user-configurable overrides disabled

Screenshot 2024-01-25 at 20 13 38

/status/overrides/{tenant} with user-configurable overrides disabled

Screenshot 2024-01-25 at 20 13 54

/status/overrides with user-configurable overrides enabled

Screenshot 2024-01-25 at 20 12 08

/status/overrides/{tenant} with user-configurable overrides enabled

Screenshot 2024-01-25 at 20 12 27

Screenshot 2024-01-25 at 20 12 43

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@kvrhdn kvrhdn marked this pull request as ready for review January 25, 2024 19:55
knylander-grafana
knylander-grafana reviewed Jan 30, 2024
View reviewed changes
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.

Doc update looks good.

mdisibio
mdisibio approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested most of this locally and LGTM. One thing I noticed is that when browsing to a non-existent tenant like /status/overrides/doesnotexist it prints the inherited settings from tenant * as expected - but is there a way to indicate that it's using the defaults and not configured specifically? Not a blocker, just curious. Also left a few nits for things that I think would improve readability. Non-blocking.

modules/overrides/overrides_tenants_http.go Outdated Show resolved Hide resolved
modules/overrides/overrides_tenants_http.go Outdated
for _, tenant := range userConfigurableOverridesManager.GetTenantIDs() {
_, hasRuntimeOverrides := tenants[tenant]

tenants[tenant] = tenantsPageTenant{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can this be simplified? Either by using *tenantsPageTenant or making use of how tenants[tenant] will return an uninitialized struct (with HasRunTimeOverrides=false)

modules/overrides/overrides_tenants_http.go Show resolved Hide resolved
modules/overrides/tenant_status.gohtml Outdated Show resolved Hide resolved
modules/overrides/user_configurable_overrides.go Outdated Show resolved Hide resolved
@kvrhdn
Copy link
Member Author

kvrhdn commented Feb 1, 2024

Tested most of this locally and LGTM. One thing I noticed is that when browsing to a non-existent tenant like /status/overrides/doesnotexist it prints the inherited settings from tenant * as expected - but is there a way to indicate that it's using the defaults and not configured specifically? Not a blocker, just curious. Also left a few nits for things that I think would improve readability. Non-blocking.

I gave this a shot and it gets a bit complicated because a tenant can be using its own runtime overrides, the wildcard (*) or the default overrides.

tempo/modules/overrides/runtime_config_overrides.go

Lines 484 to 498 in 21fde18

func (o *runtimeConfigOverridesManager) getOverridesForUser(userID string) *Overrides {
if tenantOverrides := o.tenantOverrides(); tenantOverrides != nil {
l := tenantOverrides.forUser(userID)
if l != nil {
return l
}
l = tenantOverrides.forUser(wildcardTenant)
if l != nil {
return l
}
}
return o.defaultLimits
}

I added a text line that tells you where it found the overrides, I hope this is clear enough.

If a tenant has its own runtime overrides:

Screenshot 2024-02-01 at 16 06 29

If a tenant does not have its own runtime overrides, but we have * overrides:

Screenshot 2024-02-01 at 16 06 57

If neither is set and we fall back to the overrides.defaults configuration:

Screenshot 2024-02-01 at 16 08 17

@kvrhdn kvrhdn merged commit e9afdbb into grafana:main Feb 1, 2024
15 checks passed
@kvrhdn kvrhdn deleted the kvrhdn/overrides-html branch February 1, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knylander-grafana knylander-grafana knylander-grafana left review comments

@mdisibio mdisibio mdisibio approved these changes

@jdbaldry jdbaldry Awaiting requested review from jdbaldry

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@annanay25 annanay25 Awaiting requested review from annanay25 annanay25 is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Tempo squad
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kvrhdn @mdisibio @knylander-grafana