Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65184

sakjur · 2023-03-22T14:13:53Z

What is this feature?

Adds support for querying a dashboard by its folder ID and title (folder ID is required to satisfy the UQE_org_id_folder_id_title index).
Removes support for querying a dashboard by slug (the slug index has been removed and it was only used by the two methods below).

Why do we need this feature?

#59517 changed some corner cases for slug generation and previously alerting and dashboards were using slugs to determine whether a folder already existed or not.

grafana/pkg/services/provisioning/dashboards/file_reader.go

Line 302 in b54b80f

    
           cmd := &dashboards.GetDashboardQuery{Slug: slugify.Slugify(folderName), OrgID: cfg.OrgID}

grafana/pkg/services/provisioning/alerting/rules_provisioner.go

Lines 99 to 102 in c006df3

    
           cmd := &dashboards.GetDashboardQuery{ 
        
           	Slug:  slugify.Slugify(folderName), 
        
           	OrgID: orgID, 
        
           }

Who is this feature for?

Bug fix for provisioning for users upgrading to Grafana 9.4 from an earlier version.

Which issue(s) does this PR fix?:

Fixes #64525
Fixes #64595

Special notes for your reviewer:

The steps to reproduce that I was successful in following were provided in #64525 by @ThomasFOC (🙏 thank you for the very good step-by-step instructions for reproduction), here's a tarball containing a docker-compose environment that should show the issue. Use the commented out line to alternate between the earlier version of Grafana and a later version of Grafana should trigger the issue:

grafana-issue-64525.tar.gz

For manually testing the patch, I'd recommend copying over the 1.yml provisioning file to the provisioning directory in your development environment and checking out the v9.3.8 and then a later version (I started out by testing it against 6a1d736 which was the current head of the v9.4.x branch). Validate that this is broken, and then check out this branch to validate its function.

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.
There are no known compatibility issues with older supported versions of Grafana, or plugins.
It passes the Hosted Grafana feature readiness review for observability, scalability, performance, and security.

papagian · 2023-03-22T14:35:08Z

pkg/services/provisioning/alerting/rules_provisioner.go

-		Slug:  slugify.Slugify(folderName),
-		OrgID: orgID,
+		Title:    &folderName,
+		FolderID: util.Pointer(int64(0)),


that's can be fine for now but if nested folders are enabled then we need also the parent folder information

pkg/services/provisioning/dashboards/file_reader.go

…s-get-by-title

pkg/services/dashboards/database/database.go

pkg/services/dashboards/models.go

pkg/services/dashboards/database/database_test.go

suntala · 2023-03-28T10:06:55Z

pkg/services/dashboards/database/database.go

 		if query.Title != nil {
 			dashboard.Title = *query.Title
+			mustCols = append(mustCols, "title")


Not completely familiar with MustCols yet but seems to me it would make sense to require that both title and folder_id be included if query.Title != nil. Same comment for if query.FolderID != nil

Actually I suppose we can rely on the check on line 850 if query.ID == 0 && len(query.UID) == 0 && (query.Title == nil || query.FolderID == nil) to ensure that we do include both "title" and "folder_id" in case either of them is non-nil so please disregard the earlier comment.

I would love to have this query re-written without the xorm magic

Rn, you can say “give me a dashboard with this ID and this title”, which has the same uniqueness as title + folder. We don’t need to do it like that, but that’s why those are separated. Note that as long as orgID is properly set (…out of scope for this fix) at least one unique index will be hit, but there’s nothing preventing you from adding further constraints (give me dashboard 5 but only if it’s in folder 10)

suntala

LGTM 👍

One question, does the API documentation for this need to get updated separately or is the doc comment you added going to be included somehow?

papagian

I have tested and it solves the problem (thank you for the database and the provisioning file to reproduce it)
maybe we could improve the query performance by always using the UQE_dashboard_org_id_folder_id_title index

papagian · 2023-03-28T10:06:02Z

pkg/services/dashboards/database/database.go

+		if query.FolderID != nil {
+			dashboard.FolderID = *query.FolderID
+			mustCols = append(mustCols, "folder_id")
+		}


if it's nil, I think that the UQE_dashboard_org_id_folder_id_title index won't be used
maybe it's not a big problem given that another index (IDX_dashboard_title) is used, but still has to filter out rows using where (for the org_id)

mysql> EXPLAIN SELECT * FROM dashboard WHERE org_id = 1 AND title = 'gdev dashboards'; +----+-------------+-----------+------------+------+---------------------------------------------------------------------------------------------------------------------------------------+---------------------+---------+-------+------+----------+-------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+-----------+------------+------+---------------------------------------------------------------------------------------------------------------------------------------+---------------------+---------+-------+------+----------+-------------+ | 1 | SIMPLE | dashboard | NULL | ref | UQE_dashboard_org_id_folder_id_title,UQE_dashboard_org_id_uid,IDX_dashboard_org_id,IDX_dashboard_org_id_plugin_id,IDX_dashboard_title | IDX_dashboard_title | 758 | const | 1 | 100.00 | Using where | +----+-------------+-----------+------------+------+---------------------------------------------------------------------------------------------------------------------------------------+---------------------+---------+-------+------+----------+-------------+ 1 row in set, 1 warning (0.01 sec) mysql> EXPLAIN SELECT * FROM dashboard WHERE org_id = 1 AND title = 'gdev dashboards' AND folder_id = 0; +----+-------------+-----------+------------+-------+---------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------+---------+-------------------+------+----------+-------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+-----------+------------+-------+---------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------+---------+-------------------+------+----------+-------+ | 1 | SIMPLE | dashboard | NULL | const | UQE_dashboard_org_id_folder_id_title,UQE_dashboard_org_id_uid,IDX_dashboard_org_id,IDX_dashboard_org_id_plugin_id,IDX_dashboard_title | UQE_dashboard_org_id_folder_id_title | 774 | const,const,const | 1 | 100.00 | NULL | +----+-------------+-----------+------------+-------+---------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------+---------+-------------------+------+----------+-------+ 1 row in set, 1 warning (0.01 sec)

If it’s nil, then either UID or ID is set and those have indices?

my mistake, I think that you are right

…atible way (#65184) (cherry picked from commit b210a39)

sakjur · 2023-03-28T11:25:44Z

@suntala That API documentation is just internal in the codebase, so it won’t show up anywhere. If Grafana had been compatible with pkg.go.dev you might’ve been able to see it there, but we wouldn’t have to do anything for that.

…ards compatible way (#65440) Backport from #65184 using github.com/xorcare/pointer instead of the newer util.Pointer. (cherry picked from commit b210a39) --------- Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

Dashboards: Replace get by slug with get by title

805a11a

sakjur added type/bug add to changelog backport v9.4.x Mark PR for automatic backport to v9.4.x labels Mar 22, 2023

grafanabot added the area/backend label Mar 22, 2023

papagian reviewed Mar 22, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into emil/202303/dashboard…

a1a5dd3

…s-get-by-title

sakjur added this to the 9.4.x milestone Mar 23, 2023

Merge remote-tracking branch 'origin/main' into emil/202303/dashboard…

df30d5d

…s-get-by-title

sakjur force-pushed the emil/202303/dashboards-get-by-title branch from fae2d39 to df30d5d Compare March 27, 2023 13:40

sakjur marked this pull request as ready for review March 27, 2023 14:05

sakjur requested a review from a team as a code owner March 27, 2023 14:05

sakjur requested review from mildwonkey, suntala, yangkb09 and papagian and removed request for a team March 27, 2023 14:05

tolzhabayev assigned sakjur Mar 27, 2023

suntala reviewed Mar 27, 2023

View reviewed changes

pkg/services/dashboards/database/database.go Show resolved Hide resolved

pkg/services/dashboards/models.go Show resolved Hide resolved

pkg/services/dashboards/database/database_test.go Show resolved Hide resolved

Dashboards: Add mustCols for folderID and title when present

882b0f6

sakjur requested a review from suntala March 28, 2023 09:33

sakjur mentioned this pull request Mar 28, 2023

Grafana 9.4 dashboard provisioning not working #64595

Closed

suntala reviewed Mar 28, 2023

View reviewed changes

suntala approved these changes Mar 28, 2023

View reviewed changes

papagian reviewed Mar 28, 2023

View reviewed changes

papagian approved these changes Mar 28, 2023

View reviewed changes

sakjur merged commit b210a39 into main Mar 28, 2023

sakjur deleted the emil/202303/dashboards-get-by-title branch March 28, 2023 11:24

grafanabot mentioned this pull request Mar 28, 2023

[v9.4.x] Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65440

Merged

grafanabot pushed a commit that referenced this pull request Mar 28, 2023

Dashboards: Evaluate provisioned dashboard titles in a backwards comp…

3fcb80d

…atible way (#65184) (cherry picked from commit b210a39)

zerok modified the milestones: 9.4.x, 9.4.8, 9.5.0 Apr 12, 2023

This was referenced Jun 28, 2023

Dashboard Title Slug does not support the mix of English and Chinese in 9.4.1 #66373

Closed

Slug: Use urlencoding to support non-ASCII characters #70691

Merged

suntala mentioned this pull request Aug 8, 2023

Add validation of OrgID in GetDashboard method #73064

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65184

Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65184

sakjur commented Mar 22, 2023

papagian Mar 22, 2023

suntala Mar 28, 2023 •

edited

Loading

suntala Mar 28, 2023 •

edited

Loading

papagian Mar 28, 2023

sakjur Mar 28, 2023

suntala left a comment •

edited

Loading

papagian left a comment

papagian Mar 28, 2023 •

edited

Loading

sakjur Mar 28, 2023

papagian Mar 28, 2023

sakjur commented Mar 28, 2023

	cmd := &dashboards.GetDashboardQuery{
	Slug: slugify.Slugify(folderName),
	OrgID: orgID,
	}

Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65184

Dashboards: Evaluate provisioned dashboard titles in a backwards compatible way #65184

Conversation

sakjur commented Mar 22, 2023

papagian Mar 22, 2023

Choose a reason for hiding this comment

suntala Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

suntala Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

papagian Mar 28, 2023

Choose a reason for hiding this comment

sakjur Mar 28, 2023

Choose a reason for hiding this comment

suntala left a comment • edited Loading

Choose a reason for hiding this comment

papagian left a comment

Choose a reason for hiding this comment

papagian Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

sakjur Mar 28, 2023

Choose a reason for hiding this comment

papagian Mar 28, 2023

Choose a reason for hiding this comment

sakjur commented Mar 28, 2023

suntala Mar 28, 2023 •

edited

Loading

suntala Mar 28, 2023 •

edited

Loading

suntala left a comment •

edited

Loading

papagian Mar 28, 2023 •

edited

Loading