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

feat(*): Explore Profiles app #5

Merged
merged 196 commits into from
Jul 1, 2024
Merged

feat(*): Explore Profiles app #5

merged 196 commits into from
Jul 1, 2024

Conversation

grafakus
Copy link
Contributor

@grafakus grafakus commented Jun 25, 2024

✨ Description

This PR adds a new page to the plugin, an "embedded application" based on the Scenes library. It was initially meant to replace the Tag Explorer page. It can now replace both the Tag Explorer and the Single View pages. In a near future, it will also add a Comparison exploration type.

This new app is part of the "Explore Apps" ecosystem.

🕹️ Live demo in profilesdev002.grafana-dev.net

📖 Summary of the changes

There are currently 5 exploration types:

  1. All services
  2. Single service
  3. Service labels
  4. Flame graph
  5. Favorites

All services

image

Single service

image

Service labels

image

Flame graph

image

Favorites

image

🧪 How to test?

Locally, after checking out this PR's branch

Online:

grafakus added 30 commits May 17, 2024 22:09
# Conflicts:
#	src/app/components/Routes/Routes.tsx
Copy link
Contributor

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

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

Great work! Even though I am new to scenes the structure of the code and names used made it easy to follow.

I couldn't pay full attention to all changes since the PR is on the bigger side, so I might have missed things. I added some minor remarks / questions as comments.

import { InlineSwitch } from '@grafana/ui';
import React from 'react';

export interface SceneNoDataSwitcherState extends SceneObjectState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing this here but the issue is probably elsewhere (if it is a real issue). I noticed that I can get panels to display when I select "Hide panels without data" and they have no data by applying a filter. I see the same behavior when applying the quick filter in the "All services" exploration. Is this by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be related: I've found a small bug that I've fixed in the SceneByVariableRepeaterGrid

Copy link
Contributor

@bryanhuhta bryanhuhta left a comment

Choose a reason for hiding this comment

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

This is very nice!!

I have a few thoughts that didn't seem to make sense to put any where in the PR so I'll lay them out here.


I'm not sure how we get around this, but I wish we had a way to signal moving from "All services" to "Single service" would select the first service in the grid. Perhaps not worth over-engineering, but it did trick me initially.


This is a known problem, but the fact the browser's back button is basically useless when navigating is a bit of a pain. As an example workflow:

  • I go to "All services" and find a service that is interesting
  • I click "Labels" on that grid item
  • I realize I actually wanted "Profiles"
  • I press back on my browser
  • Nothing happens sad.jpg (Well technically something happens but I'm not brought back to the same functional page I was at before)

Group by dropdown "Other labels" could have a number of options that it contains in the placeholder text.

Screenshot 2024-06-27 at 6 10 50 PM

Add hover tooltip to group by refresh to help explain what it will refresh.

Screenshot 2024-06-27 at 6 12 06 PM

@grafakus
Copy link
Contributor Author

Thank you both for the time & energy spent to provide a thorough review!
I've tried to answer all your comments and I've made some changes already. Here are a few more answers:

I'm not sure how we get around this, but I wish we had a way to signal moving from "All services" to "Single service" would select the first service in the grid. Perhaps not worth over-engineering, but it did trick me initially.

Indeed, the serviceName variable will automatically select the 1st item after loading from the /Series endpoint.
And once selected, the item sticks: you can select a service, click on different exploration types and notice that the service remains selected (e.g. from "Single service" to "Favorites" to "Flame graph"). We could prevent the automatic selection and invite the user to select a service when they land on the page initially but eventually, my feeling is that they'll get use to the automatic selection.

This is a known problem, but the fact the browser's back button is basically useless when navigating is a bit of a pain. As an example workflow:

As a temporary solution, we can add a new header action on the main timeseries in the "Service labels" and "Flame graph" views:
image

As a proper solution, I think it has been fixed in the newer versions of the Scenes library (TBC).

Add hover tooltip to group by refresh to help explain what it will refresh.

AFAIK, there's not way to customize the SceneRefreshPicker control to add a tooltip, my best effort was to add a title manually (not great):
image

@@ -13,7 +13,7 @@ const PARAM_NAMES = new Map<TargetTimeline, string[]>([
]);

const DEFAULT_TIMERANGE_VALUES = new Map<string, string>([
['from', 'now-1h'],
['from', 'now-30m'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we're going to fetch more data when landing on the page, I think that reducing the default time range is a sensible choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants