telemetry: show collected data under the toggle #8272

Pothulapati · 2022-02-17T11:21:23Z

Description

Currently, The service ping can be disabled by going into the
admin settings. Users are more likely to do that if they have
no clue what is being sent.

This PR tries to improve this experience, by showcasing the
exact data that is being sent below the option. This is possible
by creating a new TelemetryDataProvider and injecting that
both in the installation-admin-collector controller
and the gitpod-service (getTelemetryData func is added here
that can be used).

Related Issue(s)

Fixes #7891

How to test

Run yarn telepresence in /server
Head-over to the admin settings page, and view the new information below the service ping option

Release Notes

Show collected telemetry data below the telemetry toggle

Documentation

Pothulapati · 2022-02-17T11:23:13Z

components/dashboard/src/admin/Settings.tsx

                    checked={adminSettings?.sendTelemetry ?? false}
                    onChange={(evt) => actuallySetTelemetryPrefs({
                        sendTelemetry: evt.target.checked,
                    })} />
-            </PageWithSubMenu>
+                <InfoBox>{JSON.stringify(telemetryData, null, 2)}</InfoBox>


Haven't done a lot of UI yet, Hence a newbie question. What's the nicest way to display this? 😬

As a first step, I would recommend using a pre block.

Suggested change

<InfoBox>{JSON.stringify(telemetryData, null, 2)}</InfoBox>

<InfoBox><pre>{JSON.stringify(telemetryData, null, 2)}</pre></InfoBox>

before after

With this proposed change, in my opinion, it would be sufficient for a first increment (as an MVC/minimal viable change).

For a next iteration, we could think about a toggle that allows collapsing the box. For that, we should ask @gtsiolis for his always terrific input on UX design.

Updated this to use the pre block, and it already looks way better!

Hey everyone!

issue: Using the alert component[1][2], also known asInfoBox, feels a bit misleading. Also, including the payload expanded looks a bit verbose and will not scale well in the long run as we add more information encapsulated in the payload. Lastly although a minor issue, the help links pattern (Learn more, Read our Privacy Policy, etc) is used at the end of sentences without any following text. Also, yes using a monospaced font for depicting such information sounds ideal.

suggestion: What do you think of using a modal to encapsulate this information to avoid injecting this kind of verbose information in the dashboard layout?

thought: We could also 🅰️ drop the gray background and default to the white background as well as 🅱️ re-use the cancel action button if that's easier to do in the spirit of MVC (minimum viable change) using the existing modal component[1]. Ideally, in the long run, this could use a markdown component which we could either show under a toggle element or within a modal like the proposal below. 💭

Button Modal

Thanks for the ping @corneliusludmann! 🏓

Thanks for our input, @gtsiolis! I agree with your proposal. 👍

If you agree, I would like to move this part to a new issue and exclude these improvements from this PR for the following reasons:

It is functionally working now and I think it's very important that we get this into the February release.

For @Pothulapati, it would take some time to get familiar with the modal feature. Moving this into a follow-up PR would give him more time for this without jeopardizing the release (or someone else takes the new issue to implement this improvement—Tarun can decide).

Thus, my proposal would be:

Let's merge this pull request as is.

Move the UX proposals to a new issue and do the work in a follow-up PR.

@corneliusludmann Your suggestion, makes sense!

Took the liberty and created #8300.

Thanks @corneliusludmann for opening the follow up issue. Agree! Re-posting from #8125 (comment):

codecov · 2022-02-17T11:31:12Z

Codecov Report

Merging #8272 (b600fdf) into main (5f6114c) will decrease coverage by 1.13%.
The diff coverage is n/a.

❗ Current head b600fdf differs from pull request most recent head 185544a. Consider uploading reports for the commit 185544a to get more accurate results

@@            Coverage Diff             @@
##             main    #8272      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2

Flag	Coverage Δ
components-gitpod-cli-app	`11.17% <ø> (ø)`
components-installation-telemetry-app	`?`
components-local-app-app-darwin-amd64	`?`
components-local-app-app-darwin-arm64	`?`
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f6114c...185544a. Read the comment docs.

corneliusludmann

Thanks for venturing into TypeScript and the depths of Gitpod Server. 🧗 Good work. 🥳 I left a few comments.

components/dashboard/src/admin/Settings.tsx

components/server/src/installation-admin/telemetry-data-provider.ts

components/server/src/workspace/gitpod-server-impl.ts

laushinka · 2022-02-18T08:57:36Z

@Pothulapati May I have admin access please? :)

corneliusludmann · 2022-02-18T09:35:13Z

@Pothulapati @laushinka Gave you both admin access.

gtsiolis · 2022-02-18T14:56:16Z

Triggered by the comment in #8272 (comment), cross-posting a link to some relevant docs for visibility.

💡 tip: You can give give another user, including your user, admin rights with a one-line command, see relevant docs (internal).

Cc @laushinka @Pothulapati @corneliusludmann

Pothulapati · 2022-02-19T13:59:37Z

components/dashboard/src/admin/Settings.tsx

+        })();
+    });
+
+    if (!user || !user?.rolesOrPermissions?.includes('admin')) {


After this new gaurd, admin/settings does not open when the user isn't a admin like you'd expect!

components/dashboard/src/admin/Settings.tsx

Currently, The service ping can be disabled by going into the admin settings. Users are more likely to do that if they have no clue what is being sent. This PR tries to improve this experience, by showcasing the exact data that is being sent below the option. This is possible by creating a new TelemetryDataProvider and injecting that both in the `installation-admin-collector` controller and the `gitpod-service` (`getTelemetryData` func is added here that can be used). This PR also adds a gaurd to the `/admin/settings` page which was missing previously Signed-off-by: Tarun Pothulapati <[email protected]> replace cluster with gitpod instance in desc Signed-off-by: Tarun Pothulapati <[email protected]> easy nits around design and TelemetryData naming Signed-off-by: Tarun Pothulapati <[email protected]> gaurd telemetry method with admin access Signed-off-by: Tarun Pothulapati <[email protected]> replace TelemetryData at more places Signed-off-by: Tarun Pothulapati <[email protected]> gaurd access to `admin/settings` like other admin settings Signed-off-by: Tarun Pothulapati <[email protected]> call useEffect hook early Signed-off-by: Tarun Pothulapati <[email protected]>

jankeromnes · 2022-03-31T07:27:11Z

FYI, this is what this page currently looks like in staging:

As a user, I find the empty info box slightly confusing/worrying. Is this okay to ship to Self-Hosted customers as is?

Pothulapati requested a review from a team February 17, 2022 11:21

roboquat added release-note size/M labels Feb 17, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 17, 2022

Pothulapati commented Feb 17, 2022

View reviewed changes

corneliusludmann suggested changes Feb 17, 2022

View reviewed changes

components/dashboard/src/admin/Settings.tsx Outdated Show resolved Hide resolved

components/server/src/installation-admin/telemetry-data-provider.ts Outdated Show resolved Hide resolved

components/server/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved

Pothulapati force-pushed the tar/telemetry-settings-data branch from 4034e46 to 78a1efd Compare February 17, 2022 12:22

corneliusludmann mentioned this pull request Feb 18, 2022

[admin] Improve admin settings (telemetry) UX #8300

Closed

roboquat added size/L and removed size/M labels Feb 19, 2022

Pothulapati commented Feb 19, 2022

View reviewed changes

laushinka reviewed Feb 20, 2022

View reviewed changes

components/dashboard/src/admin/Settings.tsx Outdated Show resolved Hide resolved

Pothulapati force-pushed the tar/telemetry-settings-data branch from b600fdf to bfddc13 Compare February 21, 2022 10:18

Pothulapati force-pushed the tar/telemetry-settings-data branch from bfddc13 to 185544a Compare February 21, 2022 10:35

corneliusludmann approved these changes Feb 21, 2022

View reviewed changes

corneliusludmann requested a review from laushinka February 21, 2022 10:36

laushinka approved these changes Feb 21, 2022

View reviewed changes

roboquat merged commit 3be4e0b into main Feb 21, 2022

roboquat deleted the tar/telemetry-settings-data branch February 21, 2022 12:12

gtsiolis mentioned this pull request Feb 22, 2022

Telemetry payload preview is not shown on the admin dashboard #8384

Closed

gtsiolis mentioned this pull request Mar 9, 2022

Show git providers #8645

Closed

Pothulapati mentioned this pull request Apr 4, 2022

Epic: Telemetry #7498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

telemetry: show collected data under the toggle #8272

telemetry: show collected data under the toggle #8272

Pothulapati commented Feb 17, 2022 •

edited

Loading

Pothulapati Feb 17, 2022

corneliusludmann Feb 17, 2022

Pothulapati Feb 17, 2022

gtsiolis Feb 17, 2022 •

edited

Loading

corneliusludmann Feb 18, 2022

Pothulapati Feb 18, 2022

corneliusludmann Feb 18, 2022

gtsiolis Feb 18, 2022

codecov bot commented Feb 17, 2022 •

edited

Loading

corneliusludmann left a comment

laushinka commented Feb 18, 2022

corneliusludmann commented Feb 18, 2022

gtsiolis commented Feb 18, 2022 •

edited

Loading

Pothulapati Feb 19, 2022

jankeromnes commented Mar 31, 2022 •

edited

Loading

	<InfoBox>{JSON.stringify(telemetryData, null, 2)}</InfoBox>
	<InfoBox><pre>{JSON.stringify(telemetryData, null, 2)}</pre></InfoBox>

telemetry: show collected data under the toggle #8272

telemetry: show collected data under the toggle #8272

Conversation

Pothulapati commented Feb 17, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2022 • edited Loading

Codecov Report

corneliusludmann left a comment

Choose a reason for hiding this comment

laushinka commented Feb 18, 2022

corneliusludmann commented Feb 18, 2022

gtsiolis commented Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

jankeromnes commented Mar 31, 2022 • edited Loading

Pothulapati commented Feb 17, 2022 •

edited

Loading

gtsiolis Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading

gtsiolis commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Mar 31, 2022 •

edited

Loading