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

Dashboards: Add dashboard embed route #69596

Merged
merged 38 commits into from
Jul 6, 2023
Merged

Dashboards: Add dashboard embed route #69596

merged 38 commits into from
Jul 6, 2023

Conversation

Clarity-89
Copy link
Contributor

@Clarity-89 Clarity-89 commented Jun 6, 2023

What is this feature?
Add separate route and component for embedding a dashboard externally. The component loads a dashboard JSON from a specified endpoint and initializes a dashboard page. After dashboard is edited, its JSON is sent to a separate POST endpoint on save.

This is required for the VS Code plugin feature and is behind the dashboardEmbed feature toggle.

Design doc.

Steps for testing.

@Clarity-89 Clarity-89 self-assigned this Jun 6, 2023
@grafanabot grafanabot added area/backend area/frontend type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project labels Jun 6, 2023
# Conflicts:
#	packages/grafana-data/src/types/featureToggles.gen.ts
#	pkg/services/featuremgmt/registry.go
#	pkg/services/featuremgmt/toggles_gen.csv
#	pkg/services/featuremgmt/toggles_gen.go
# Conflicts:
#	pkg/services/featuremgmt/toggles_gen.csv
@polibb polibb self-requested a review June 20, 2023 10:53
@polibb
Copy link
Contributor

polibb commented Jun 21, 2023

I can't get it running as expected: did all the steps and cannot open the dashboard in the web view in the VSCode extension. Maybe I'm doing something different? I attached some screenshots in the Slack thread with the steps.

@natellium natellium added the team/grafana-dashboards Dashboards squad label Jun 23, 2023
# Conflicts:
#	docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md
Copy link
Contributor

@kaydelaney kaydelaney left a comment

Choose a reason for hiding this comment

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

Tested and works fine. I get a "Invalid dashboard UID in annotation request" notification when I edit the dashboard in vs code, but maybe that's a known issue

@Clarity-89
Copy link
Contributor Author

Tested and works fine. I get a "Invalid dashboard UID in annotation request" notification when I edit the dashboard in vs code, but maybe that's a known issue

Yeah, that happens if you don't have the dashboard with the same UID as in JSON in your Grafana. Could be also fixed by manually removing the UID in the json file.

# Conflicts:
#	docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md
#	packages/grafana-data/src/types/featureToggles.gen.ts
#	pkg/services/featuremgmt/registry.go
#	pkg/services/featuremgmt/toggles_gen.csv
#	pkg/services/featuremgmt/toggles_gen.go
Copy link
Contributor

@polibb polibb left a comment

Choose a reason for hiding this comment

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

Love the separation of concerns!

Clarity-89 and others added 2 commits July 6, 2023 16:50
…eDashboardForm.tsx

Co-authored-by: Polina Boneva <13227501+polibb@users.noreply.github.com>
@Clarity-89 Clarity-89 merged commit 420b19e into main Jul 6, 2023
11 checks passed
@Clarity-89 Clarity-89 deleted the dashboard-embed branch July 6, 2023 14:43
@Clarity-89 Clarity-89 modified the milestones: 10.2.x, 10.1.x Jul 13, 2023
polibb added a commit that referenced this pull request Jul 14, 2023
* Dashboard embed: Set up route

* Dashboard embed: Cleanup

* Dashboard embed: Separate routes

* Dashboard embed: Render dashboard page

* Dashboard embed: Add toolbar

* Dashboard embed: Send JSON on save

* Dashboard embed: Add JSON param

* Dashboard embed: Make the dashboard editable

* Fix sending dashboard to remote server

* Add notifications

* Add "dashboardEmbed" feature toggle

* Use the toggle

* Update toggles

* Add toggle on backend

* Add get JSON endpoint

* Add drawer

* Close drawer on success

* Update toggles

* Cleanup

* Update toggle

* Allow embedding for the d-embed url

* Allow embedding via custom X-Allow-Embedding header

* Use callbackUrl

* Cleanup

* Update public/app/features/dashboard/containers/EmbeddedDashboardPage.tsx

Co-authored-by: kay delaney <45561153+kaydelaney@users.noreply.github.com>

* Use theme for spacing

* Update toggles

* Update public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardForm.tsx

Co-authored-by: Polina Boneva <13227501+polibb@users.noreply.github.com>

* Add select data source modal

---------

Co-authored-by: kay delaney <45561153+kaydelaney@users.noreply.github.com>
Co-authored-by: Polina Boneva <13227501+polibb@users.noreply.github.com>
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Comment on lines +48 to +54
const callbackUrl = queryParams.callbackUrl;

if (!callbackUrl) {
throw new Error('No callback URL provided');
}
getBackendSrv()
.get(`${callbackUrl}/load-dashboard`)
Copy link
Member

Choose a reason for hiding this comment

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

@Clarity-89
I think this could be a pretty big security issue loading a resource from a URL provided via query params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're still working out all possible security issues, so I'll add this to the list.

};

return (
<PageToolbar title={dashboard.title} buttonOverflowAlignment="right" className={styles.toolbar}>
Copy link
Member

Choose a reason for hiding this comment

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

This is using the deprecated old PageToolbar component from the old nav, not sure we want to keep using it

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

I am a little bit dismayed by the duplication this requires for an experimental feature

@Clarity-89
Copy link
Contributor Author

I am a little bit dismayed by the duplication this requires for an experimental feature

I've tried to reuse the existing functionality as much as possible, but because we needed to add custom logic pretty deep in the component tree, it was not always possible.

@torkelo
Copy link
Member

torkelo commented Aug 15, 2023

@Clarity-89 yea, I get that. Not easy to do this differently. We ended up with a similar solution for PublicDashboardPage, but with the addition of panel editing, and more I am worried about maintaining so many different dashboard pages. Feels like there could be a better solution if we had the right hooks / extension points but not sure

@Clarity-89
Copy link
Contributor Author

@Clarity-89 yea, I get that. Not easy to do this differently. We ended up with a similar solution for PublicDashboardPage, but with the addition of panel editing, and more I am worried about maintaining so many different dashboard pages. Feels like there could be a better solution if we had the right hooks / extension points but not sure

Yeah, also the dashboard page is still a class based component, so it's not possible to extend with hooks.

I get the concern and agree it's not a good idea to add similar pages for dashboards, especially when there's a major rework in place. I'm exploring some other options now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend area/frontend no-backport Skip backport of PR team/grafana-dashboards Dashboards squad type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants