Sidecar: Implement basic integration with Explore Logs #102

aocenas · 2024-07-22T16:13:50Z

Add a POC code to test sidecar functionality across Explore apps.

This ties in with the PR on the Logs side grafana/explore-logs#828 and should allow this functionality:

explore-app-sidecar.mp4

This is to showcase some basics of what is possible but isn't meant as a final functionality. It is tied to appSidecar feature toggle and is meant only to be enabled internally for now.

adrapereira · 2024-09-06T15:14:40Z

@aocenas This can be closed right? And the linked PR can likely be closed as well since there was a change merged last week to remove the singleton.

ifrost · 2024-09-09T13:07:52Z

@aocenas We'll close it for now but feel free to reopen if it's still needed.

ifrost

Looks really nice!

What's the plan after this gets merged? Enable it internally to get some feedback?

I think it's okay to push it for testing but I would love to see how UX could be improved, e.g.:

it's not clear you need to close the side car to get rid of it, and on a small screen I had to scroll to the right to see the close button
the state/history is saved in local storage so it nicely stays on the screen on refresh but it creates an impression you can share the link with others which is not true

It'd be also great to see how API evolves when we scale it to multiple apps:

how to simplify creating hooks between multiple apps
how to handle multiple types of context between apps

I guess we can figure it out when we add more apps to the mix but to do this we need to get some stuff merged :D

ifrost · 2024-11-05T11:23:43Z

src/pages/Explore/TraceExplorationPage.tsx

  const initialDs = localStorage.getItem(DATASOURCE_LS_KEY) || '';
-  const [exploration] = useState(newTracesExploration(initialDs));
+  const [exploration] = useState(newTracesExploration(initialDs, getInitialFilters(sidecarContext.initialContext)));


Should we clear the context after the app is opened? At the moment if the side panel is not closed with "x" it just stays open. It's stored in the local storage so it opens in new tabs as well:

Screen.Recording.2024-11-05.at.12.16.25.mov

Also on the video somehow the range was not set (it shows 1970...) but I can't reproduce it anymore 😅

So this is the kind of feedback I hope for after this is in Ops. The thing is the persistence was added because the Investigations app which makes sense to be kept "alive" as you can add multiple things to it from multiple places. Right now not sure what exactly is the cases where that is not wanted. Maybe we can have it somehow configurable but not sure what exactly the options should be right now.

I think this can go like this for testing internally, though it would be nice to make the "X" button more visible somehow or maybe sticky for now? I'm worried some user may be not sure how to get rid of this. Also it may be confusing as it may show up on any tab after it's open, right?

I made some changes so the width of the header with the X button is fixed so it never goes out of sight. Later will probably try to design it a bit better. For example I assume it needs some sort of additional action, like "maximize" or "open in separate tab" etc

Also it may be confusing as it may show up on any tab after it's open, right?

Yes the state is persisted. Again not sure what is the best approach here, at some point I will make it configurable as part of also making the URL work properly, so that explore apps will just be persisted in the URL and work sort of normally and having a non URL but persisted in local storage state as some sort of opt in for specific apps.

ifrost · 2024-11-05T11:25:29Z

src/pages/Explore/TraceExplorationPage.tsx

+});
+
+/** Because the context comes from a different app plugin we cannot really count on it being the correct type even if
+ * it was typed, so it is safer to do runtime parsing here. It also can come from different app extensions and at this


It also can come from different app extensions...

Does it mean other apps could potentially call sidecarServiceSingleton_EXPERIMENTAL?.openApp(anyPluginIdHere, helpers.context); providing any id in anyPluginIdHere?

Yes, it's not ideal and it's also just temporary. The reason for it is that otherwise you have to bound openApp to current app plugin which can only been done by propagating it through some function args or props, but that then means this would already change existing API which means less space for exploration and changes later on.

Once this is good enough the sidecarServiceSingleton_EXPERIMENTAL APIs will probably be moved elsewhere to accommodate this.

ifrost · 2024-11-05T11:28:21Z

src/plugin.json

+  "extensions": {
+    "addedLinks": [
+      {
+        "targets": ["grafana-lokiexplore-app/toolbar-open-related/v1"],


This creates a link to very specific place in the UI. Would it mean we need to create multiple links to other apps or we're also planning to make more generic extension point that apps could reuse? e.g.

extension point grafana/side-car

the app with the button calls <SideCarDropDown ... > that renders all registered hooks

Hmm not sure exactly what you mean. Like extension point is always a specific place in the UI by design. You could do something like grafana/side-car and then have that extension point on multiple places but then you loose the information about the location which may be important (ie whether it's in logs or metrics app), so you probably again end up with some way to differentiate them which could already be the ID.

Like extension point is always a specific place in the UI by design.

The confusing part to me is that if we have specific link and we know where the action is coming from - why do we need to figure out the context and check the schema? 🤔

Oh, okay, I just saw you comment below "Regarding the context check, I think that will be necessary....". Fair enough. I thought the "guessing" logic would be to support multiple inputs not assert the expected contract 👍

ifrost · 2024-11-05T11:40:27Z

src/pages/Explore/TraceExplorationPage.tsx

+ * normalization of the filters or smart guesses when there are differences.
+ * @param context
+ */
+function getInitialFilters(context: unknown): AdHocVariableFilter[] | undefined {


Is the plan to use AdHocFilters as common contract for context between the apps? Or it's assumed that if I'm creating a hook for grafana-lokiexplore-app/toolbar-open-related/v1" I should check what type of context it provides?

Those are 2 possibilities, not sure which makes more sense.

Having a sync button on AdHocFilters could be nice (in similar way that it is right now with timePickers) but I think you may want to add some additional processing as lot's of times the labels aren't exactly the same.

Leave it to the application, so they can use some other way to stay sync (extension for that or some event bus). I think this depends on whether we can find good enough standardization or not.

Regarding the context check, I think that will be necessary. Even if we add some common types in some grafana/* package you could use, it still does not make sense to blindly trust the data, because this is a boundary that you just cannot compile time check if the types are right in the same way you cannot check network request at compile times.

aocenas · 2024-11-05T13:39:15Z

What's the plan after this gets merged? Enable it internally to get some feedback?

Yes, have it on ops, tell about it to people, get some feedback and data.

it's not clear you need to close the side car to get rid of it, and on a small screen I had to scroll to the right to see the close button

Agree, need to investigate a bit how to make some better chrome for it.

the state/history is saved in local storage so it nicely stays on the screen on refresh but it creates an impression you can share the link with others which is not true

Have a plan to add better URL support soon. This seems to be something that isn't needed for apps like the Investigations but needed for something like this so didn't prioritize it yet.

how to simplify creating hooks between multiple apps

Yeah no idea for now. It is a bit tedious, but that seems unrelated to sidecar. If we want to add more and more extensions the dev experience should probably be nicer in general.

how to handle multiple types of context between apps

As mentioned here #102 (comment) the context will be a bit tricky. We can probably have some common types but I guess that would need to be both a type and a parse/validate function. At the same time 3rd party apps probably should not be forced to go through some grafana/* package PR to be able to function, creating a centralised bottleneck, so some level of uncertainty I think will be always there.

joey-grafana

Looking good, beautiful addition 👍

This is good to get the ball rolling. As you say we can test, get feedback and improve from there.

Apart from Piotr's comments, going to specific parts of Explore Traces such as errored traces list or comparison view would be cool, based on the context in Explore Logs. That's something that can be incorporated based on feedback and as this is iterated, so not necessary now.

It's behind a toggle, so we can be safe :D about this, perhaps just enable in dev/ops for now to get feedback but ultimately up to you.

joey-grafana · 2024-11-08T15:02:26Z

Also, of course, need to fix compatibility issue in the build before merge.

joey-grafana · 2024-11-08T15:37:07Z

It actually messes up the page title when sidecar is open

aocenas · 2024-11-11T10:13:46Z

Hey @joey-grafana thanks for the review. I already have a separate PR with some fixes/improvements but didn't want to make this PR too big so I separated them #264, will also take a look at the title in that separate PR.

Clarity-89 · 2024-11-11T14:58:02Z

src/components/Routes/Routes.tsx

 import { TraceExplorationPage } from '../../pages/Explore';
 import { PLUGIN_BASE_URL, ROUTES } from 'utils/shared';

 export const Routes = () => {
  return (
    <Switch>
-      <Route path={prefixRoute(`${ROUTES.Explore}`)} component={TraceExplorationPage} />
+      <CompatRoute path={prefixRoute(`${ROUTES.Explore}`)} component={TraceExplorationPage} />
      <Redirect to={prefixRoute(ROUTES.Explore)} />
    </Switch>
  );


I checked and it doesn't seem like we need this file or react-router here at all. We can just render TraceExplorationPage inside App instead of the Routes and it should work. The actual plugin routes are registered via plugin.json's includes property, so I think this explicit route declaration is redundant.

Yeah could be but don't want to do too many unrelated changes here so will leave that for traces team to handle separately.

joey-grafana · 2024-11-12T09:41:59Z

You'll also want to update the grafanaDependency in plugin.json to >=11.3.0.

Other than that, and the other PR you created for updates, this looks good for v1 so we can get some feedback and iterate from there.

Use non singleton UrlSyncManager

b95506e

aocenas mentioned this pull request Jul 22, 2024

Make UrlSyncManager non singleton grafana/scenes#841

Closed

ifrost closed this Sep 9, 2024

Remove getUrlSyncManager().getUrlState

6e31c3c

aocenas mentioned this pull request Sep 12, 2024

Use locationService from context in UrlSyncManager grafana/scenes#899

Merged

aocenas reopened this Sep 25, 2024

aocenas added 3 commits September 25, 2024 15:08

Merge branch 'main' into aocenas/scenes-updates-sidecar

fbf5932

Use scenes canary build

21a51d3

Add example extension

b4e4e46

aocenas mentioned this pull request Oct 16, 2024

Add conditional extension point for testing sidecar functionality grafana/explore-logs#828

Merged

aocenas added 5 commits October 23, 2024 15:53

Add filter passing from extension

67718cb

Merge branch 'main' into aocenas/scenes-updates-sidecar

561bf8a

Use non canary version of scenes

fedbe10

Update the extension point id

111a2fa

Merge branch 'main' into aocenas/scenes-updates-sidecar

85c91bf

aocenas marked this pull request as ready for review October 31, 2024 11:04

aocenas changed the title ~~Use non singleton UrlSyncManager~~ Sidecar: Implement basic integration with Explore Logs Oct 31, 2024

Add spelling ignore

197bb5e

aocenas requested a review from a team October 31, 2024 14:56

ifrost reviewed Nov 5, 2024

View reviewed changes

joey-grafana approved these changes Nov 8, 2024

View reviewed changes

Update grafana/* packages

f673045

This was referenced Nov 11, 2024

Sidecar: Implement POC for explore logs to traces grafana/grafana#96218

Closed

Sidecar: Add app extensions in Explore Traces using the sidecar API grafana/grafana#96221

Open

Use compatRoute

f4cd84a

Clarity-89 reviewed Nov 11, 2024

View reviewed changes

Update router stuff

11228dd

Update grafanaDependency

78aac1a

aocenas merged commit 636310f into main Nov 12, 2024
4 checks passed

aocenas deleted the aocenas/scenes-updates-sidecar branch November 12, 2024 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidecar: Implement basic integration with Explore Logs #102

Sidecar: Implement basic integration with Explore Logs #102

aocenas commented Jul 22, 2024 •

edited

Loading

adrapereira commented Sep 6, 2024

ifrost commented Sep 9, 2024

ifrost left a comment

ifrost Nov 5, 2024

aocenas Nov 5, 2024

ifrost Nov 8, 2024

aocenas Nov 8, 2024

aocenas Nov 8, 2024

ifrost Nov 5, 2024

aocenas Nov 5, 2024 •

edited

Loading

ifrost Nov 5, 2024

aocenas Nov 5, 2024

ifrost Nov 8, 2024

ifrost Nov 8, 2024

ifrost Nov 5, 2024

aocenas Nov 5, 2024

aocenas commented Nov 5, 2024

joey-grafana left a comment

joey-grafana commented Nov 8, 2024

joey-grafana commented Nov 8, 2024

aocenas commented Nov 11, 2024

Clarity-89 Nov 11, 2024

aocenas Nov 11, 2024 •

edited

Loading

joey-grafana commented Nov 12, 2024

Sidecar: Implement basic integration with Explore Logs #102

Sidecar: Implement basic integration with Explore Logs #102

Conversation

aocenas commented Jul 22, 2024 • edited Loading

adrapereira commented Sep 6, 2024

ifrost commented Sep 9, 2024

ifrost left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aocenas Nov 5, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aocenas commented Nov 5, 2024

joey-grafana left a comment

Choose a reason for hiding this comment

joey-grafana commented Nov 8, 2024

joey-grafana commented Nov 8, 2024

aocenas commented Nov 11, 2024

Choose a reason for hiding this comment

aocenas Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

joey-grafana commented Nov 12, 2024

aocenas commented Jul 22, 2024 •

edited

Loading

aocenas Nov 5, 2024 •

edited

Loading

aocenas Nov 11, 2024 •

edited

Loading