Skip to content

[One Discover] Pass app state and global state to locator when redirecting from /stream path#215867

Merged
rStelmach merged 21 commits intoelastic:mainfrom
rStelmach:212429-one-discover-fix-redirects-from-log-stream-and-logs-explorer-to-discover
May 23, 2025
Merged

[One Discover] Pass app state and global state to locator when redirecting from /stream path#215867
rStelmach merged 21 commits intoelastic:mainfrom
rStelmach:212429-one-discover-fix-redirects-from-log-stream-and-logs-explorer-to-discover

Conversation

@rStelmach
Copy link
Contributor

@rStelmach rStelmach commented Mar 25, 2025 •
edited by kibanamachine
Loading

Modified the /stream route and redirect handlers from Logs Explorer to carry state parameters when redirecting between:

Logs Stream → Logs Explorer

Logs Stream → Discover

Logs Explorer → Discover
@rStelmach rStelmach added bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application release_note:fix Team:obs-onboarding Observability Onboarding Team backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Mar 25, 2025
@rStelmach rStelmach requested review from a team as code owners March 25, 2025 13:20
@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 25, 2025

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)
@rStelmach rStelmach linked an issue Mar 25, 2025 that may be closed by this pull request
[One Discover] Fix redirects from Log Stream and Logs Explorer to Discover #212429
Closed
davismcphee
davismcphee reviewed Mar 25, 2025
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Just left a few questions and comments from the Data Discovery side.
.../shared/discover/public/application/main/state_management/discover_global_state_container.ts Outdated
Comment on lines 30 to 40
export interface DiscoverGlobalState {
filters: Filter[];
refreshInterval: {
pause: boolean;
value: number;
};
time: {
from: string;
to: string;
};
}
Copy link
Contributor

@davismcphee davismcphee Mar 25, 2025

Choose a reason for hiding this comment

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

The type we use for our global state is QueryState from @kbn/data-plugin/common. Could that be used directly instead of creating a dedicated type for this redirect? It's also not quite accurate since these props are optional in QueryState and Discover.
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Outdated
Comment on lines 86 to 90
const g = searchParams.get('_g') || '';
const a = searchParams.get('_a') || '';

const gState = (safeDecode(g) || {}) as unknown as DiscoverGlobalState;
const aState = (safeDecode(a) || {}) as unknown as DiscoverAppState;
Copy link
Contributor

@davismcphee davismcphee Mar 25, 2025

Choose a reason for hiding this comment

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

I'm not familiar with this code, but is it expected that a user will be sent to /stream with Discover params in the URL? Mainly wondering if these should be typed as something else since I'm surprised to see Discover's internal state types used here.
Copy link
Contributor Author

@rStelmach rStelmach Mar 26, 2025

Choose a reason for hiding this comment

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

You are right, I will be making modifications to my current approach and I'll let you know when it's done
Copy link
Contributor Author

@rStelmach rStelmach Mar 28, 2025

Choose a reason for hiding this comment

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

I did some changes that should properly redirect from logs/stream to logs explorer (in older versions) as well as redirecting directly to discover.
I also handled redirecting from Logs explorer to Discover.
Copy link
Contributor

@davismcphee davismcphee Mar 28, 2025

Choose a reason for hiding this comment

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

Thanks for the updates! This definitely looks more in line with how I'd expect this to be handled 👍
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Outdated
? (aState.dataSource as DataViewDataSource).dataViewId
: '';

const locatorParams = {
Copy link
Contributor

@davismcphee davismcphee Mar 25, 2025

Choose a reason for hiding this comment

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

Just a couple of small nits/suggestions:

  • This could be typed as DiscoverAppLocatorParams if you want better typings here.
  • All DiscoverAppLocatorParams props are optional, so you could skip the default values here and just rely on Discover defaults if preferred.
  • There's also a strongly typed version of the locator available on the Discover start contract that could be used for this.
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Outdated
sort: Array.isArray(aState.sort) ? aState.sort : [['@timestamp', 'desc']],
};

share.url.locators.get(DISCOVER_APP_LOCATOR)?.navigate(locatorParams);
Copy link
Contributor

@davismcphee davismcphee Mar 25, 2025

Choose a reason for hiding this comment

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

Suggested change
share.url.locators.get(DISCOVER_APP_LOCATOR)?.navigate(locatorParams);
share.url.locators.get<DiscoverAppLocatorParams>(DISCOVER_APP_LOCATOR)?.navigate(locatorParams);

Nit: If nothing else, I'd recommend typing the locator when retrieving it here.
davismcphee
davismcphee approved these changes Mar 28, 2025
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

With the latest updates there are no changes in our area, so it technically doesn't need our approval, but approving anyway since I see no issues from the Discover side now. Left a small nit about constructing the Discover URL, but nothing blocking on our end.
.../solutions/observability/plugins/observability_logs_explorer/public/redirect_to_discover.tsx Outdated
Comment on lines 53 to 57
const gEncoded = encode(gState);
const aEncoded = encode(aState);

const newSearch = `#/?_g=${gEncoded}&_a=${aEncoded}`;
const path = `${location.pathname}${newSearch}`;
Copy link
Contributor

@davismcphee davismcphee Mar 28, 2025

Choose a reason for hiding this comment

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

Nit: These query params are part of our public API so this is technically safe to do, although in general I'd recommend opting for the Discover locator whenever possible to avoid manually constructing them.
Copy link
Member

@weltenwort weltenwort Apr 10, 2025

Choose a reason for hiding this comment

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

Agreed - and although navigating like this directly in the component was already introduced in an earlier PR, I wonder if we shouldn't do this cleanly and use the proper locator in a proper effect. Does that make sense?
Copy link
Contributor

@davismcphee davismcphee Apr 11, 2025

Choose a reason for hiding this comment

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

It does on my end for sure!
Copy link
Contributor Author

@rStelmach rStelmach Apr 11, 2025

Choose a reason for hiding this comment

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

Only thing I was wondering if it would somehow impact the performance of a plugin.
Correct me if I'm wrong but in order to use locator we would have to wrap the plugin with Kibana Context Provider. I just wasn't sure if it would affect the plugin anyhow
Copy link
Member

@weltenwort weltenwort Apr 11, 2025

Choose a reason for hiding this comment

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

valid point - but in order to access the locator you only need the share plugin, which you could pass to the renderDiscoverRedirect in the plugin.ts in addition to coreStart:

kibana/x-pack/solutions/observability/plugins/observability_logs_explorer/public/plugin.ts

Lines 38 to 40 in e0f106f

const [coreStart] = await core.getStartServices();
const { renderDiscoverRedirect } = await import('./redirect_to_discover');
return renderDiscoverRedirect(coreStart, appMountParams);

Copy link
Contributor Author

@rStelmach rStelmach Apr 11, 2025

Choose a reason for hiding this comment

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

oh that's the magic I didn't know 😄
Will apply.
Copy link
Contributor

@davismcphee davismcphee Apr 11, 2025

Choose a reason for hiding this comment

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

You don't necessarily need to wrap it in the context provider, although I don't think you'd find any performance impact if you did. It looks like the plugin already takes a dependency on the discover plugin, it would just need to be added to ObservabilityLogsExplorerStartDeps and passed down when mounting the app here:

kibana/x-pack/solutions/observability/plugins/observability_logs_explorer/public/plugin.ts

Lines 44 to 54 in e0f106f

// App used solely to redirect from "/app/observability-log-explorer" to "/app/discover"
core.application.register({
id: 'observability-log-explorer',
title,
visibleIn: [],
mount: async (appMountParams) => {
const [coreStart] = await core.getStartServices();
const { renderDiscoverRedirect } = await import('./redirect_to_discover');
return renderDiscoverRedirect(coreStart, appMountParams);
},
});

Something along these lines:

// x-pack/solutions/observability/plugins/observability_logs_explorer/public/types.ts
import { DiscoverStart } from '@kbn/discover-plugin/public';

export interface ObservabilityLogsExplorerStartDeps {
  discover: DiscoverStart;
}

// x-pack/solutions/observability/plugins/observability_logs_explorer/public/plugin.ts
core.application.register({
  id: 'observability-log-explorer',
  title,
  visibleIn: [],
  mount: async (appMountParams) => {
    const [coreStart, { discover }] = await core.getStartServices();
    const { renderDiscoverRedirect } = await import('./redirect_to_discover');
    return renderDiscoverRedirect(coreStart, discover, appMountParams);
  },
});

// x-pack/solutions/observability/plugins/observability_logs_explorer/public/redirect_to_discover.tsx
export const renderDiscoverRedirect = (
  core: CoreStart,
  discover: DiscoverStart,
  appParams: AppMountParameters
) => {
  ReactDOM.render(
    <Router history={appParams.history}>
      <DiscoverRedirect core={core} discover={discover} />
    </Router>,
    appParams.element
  );

  return () => {
    ReactDOM.unmountComponentAtNode(appParams.element);
  };
};

export const DiscoverRedirect = ({
  core,
  discover,
}: {
  core: CoreStart;
  discover: DiscoverStart;
}) => {
  ...
  discover.locator?.navigate({ ...params });
  ...
};
Copy link
Contributor

@davismcphee davismcphee Apr 11, 2025

Choose a reason for hiding this comment

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

Oops, already had this page open when @weltenwort commented and didn't realize he already answered. Just noticed when checking my email now, ignore me 😅
Copy link
Contributor Author

@rStelmach rStelmach Apr 11, 2025

Choose a reason for hiding this comment

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

I took the code from your comment,so it was definitely helpful 😃
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Outdated
Comment on lines 86 to 90
const g = searchParams.get('_g') || '';
const a = searchParams.get('_a') || '';

const gState = (safeDecode(g) || {}) as unknown as DiscoverGlobalState;
const aState = (safeDecode(a) || {}) as unknown as DiscoverAppState;
Copy link
Contributor

@davismcphee davismcphee Mar 28, 2025

Choose a reason for hiding this comment

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

Thanks for the updates! This definitely looks more in line with how I'd expect this to be handled 👍
@weltenwort weltenwort self-requested a review April 9, 2025 19:36
weltenwort
weltenwort reviewed Apr 10, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The redirect from the stream to discover seems to work. But the redirect from the logs explorer to Discover failed with this error in the sidebar when I had previously added the "message" column:

image

The full stack trace was:

atalReactError: Objects are not valid as a React child (found: object with keys {field, type}). If you meant to render a collection of children, use an array instead.
    at throwOnInvalidObjectType (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:55694:9)
    at reconcileChildFibers (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:56635:7)
    at reconcileChildren (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:59969:28)
    at updateHostComponent (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:60726:3)
    at beginWork (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:62420:14)
    at beginWork$1 (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:68228:14)
    at performUnitOfWork (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:67359:12)
    at workLoopSync (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:67268:5)
    at renderRootSync (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:67236:7)
    at performSyncWorkOnRoot (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:66887:20)


The above error occurred in span:
    at span
    at EuiHighlight (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:283252:23)
    at span
    at span
    at button
    at div
    at FieldButton (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.12.js:75:3)
    at FieldItemButton (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.11.js:489:3)
    at div
    at DraggableImpl (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.13.js:161:3)
    at Draggable (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.13.js:122:3)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiPopover (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:224541:86)
    at FieldPopover (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.11.js:888:3)
    at UnifiedFieldListItemComponent (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.28.js:202:3)
    at li
    at ul
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiResizeObserver (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:214660:86)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiAccordionChildren (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:215684:19)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiAccordionClass (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:212640:86)
    at Render (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:165819:17)
    at InnerFieldsAccordion (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.23.js:278:3)
    at div
    at div
    at InnerFieldListGrouped (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.23.js:58:3)
    at Suspense
    at WrappedFieldListGrouped
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexItemInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228892:23)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexGroupInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228473:24)
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at FieldList (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.11.js:714:21)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexItemInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228892:23)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexGroupInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228473:24)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiPageSidebar (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:342135:23)
    at UnifiedFieldListSidebarComponent (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.28.js:525:3)
    at EuiHideFor (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:318050:23)
    at UnifiedFieldListSidebarContainer (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.28.js:798:5)
    at Suspense
    at ErrorBoundaryInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:230973:5)
    at KibanaErrorBoundary (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:231033:79)
    at KibanaErrorBoundaryProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:230691:3)
    at http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.plugin.js:5172:20
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexItemInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228892:23)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiFlexGroupInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:228473:24)
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at DiscoverSidebarResponsive (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:3509:136)
    at InPortal (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:12637:28)
    at DiscoverResizableLayout (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:1517:3)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiPageBody (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:341325:23)
    at div
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at EuiPage (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:341108:23)
    at http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190088:66
    at DiscoverLayout (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:973:3)
    at ChildDragDropProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.13.js:2306:3)
    at RootDragDropProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.13.js:2233:3)
    at DiscoverMainApp (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:6393:5)
    at RuntimeStateProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.3.js:1005:3)
    at BaseAppWrapper (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.9.js:2808:3)
    at Provider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:37526:20)
    at InternalStateProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.3.js:781:3)
    at DiscoverMainProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:9155:3)
    at DiscoverMainRoute (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.2.js:6546:3)
    at Route (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:414922:29)
    at Route (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:234902:3)
    at Switch (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:415124:29)
    at Routes (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:235261:3)
    at DiscoverRoutes (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.8.js:2829:3)
    at RenderedRoute (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:417645:5)
    at Routes (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:418206:5)
    at Router (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:418144:15)
    at CompatRouter (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:416999:5)
    at Router (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:414541:30)
    at Router (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:235218:3)
    at EuiErrorBoundary (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:318924:86)
    at Provider (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/kibanaReact/1.0.0/kibanaReact.plugin.js:422:15)
    at DiscoverRouter (http://localhost:5601/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/discover.chunk.8.js:2813:3)
    at ErrorBoundaryInternal (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:230973:5)
    at KibanaErrorBoundary (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:231033:79)
    at KibanaErrorBoundaryProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:230691:3)
    at EuiContext (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:210801:24)
    at IntlProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:10220:47)
    at I18nProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:10721:3)
    at I18nContext (http://localhost:5601/XXXXXXXXXXXX/bundles/core/core.entry.js:14124:9)
    at EuiComponentDefaultsProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:215077:36)
    at CurrentEuiBreakpointProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:165581:23)
    at ThemeProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:190151:50)
    at EuiEmotionThemeProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:193786:23)
    at EuiThemeMemoizedStylesProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:193836:23)
    at EuiThemeProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:193163:22)
    at EuiSystemColorModeProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:343151:23)
    at EuiCacheProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:343093:20)
    at EuiProviderNestedCheck (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:343241:23)
    at EuiProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:342937:25)
    at KibanaEuiProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:234326:3)
    at KibanaRootContextProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:234217:3)
    at KibanaRenderContextProvider (http://localhost:5601/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src.js:234612:3)

Seems like we need to translate the column data structures and make sure the right dataview is used in Discover.
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Outdated
Comment on lines 105 to 107
{i18n.translate('xpack.infra.logsPageContent.routes.LegacyRendersAndLabel', {
defaultMessage: '// Legacy renders and redirects',
})}
Copy link
Member

@weltenwort weltenwort Apr 10, 2025

Choose a reason for hiding this comment

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

Looks like this comment would not have been interpreted as such by the JSX compiler before and the linter was a bit over-zealous here 😁 How about this?

Suggested change
{i18n.translate('xpack.infra.logsPageContent.routes.LegacyRendersAndLabel', {
defaultMessage: '// Legacy renders and redirects',
})}
{/* Legacy renders and redirects */}
.../solutions/observability/plugins/observability_logs_explorer/public/redirect_to_discover.tsx Outdated
const path = `${location.pathname}${location.search}`;
const searchParams = new URLSearchParams(location.search);
const pageStateEncoded = searchParams.get('pageState') || '';
const pageState = (safeDecode(pageStateEncoded) || {}) as {
Copy link
Member

@weltenwort weltenwort Apr 10, 2025

Choose a reason for hiding this comment

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

This is some impressive reverse-engineering of the url state. If we wanted to preserve type safety and ensure full compatibility we could resurrect the actual schema used for the url state from the 8.x branch before the removal of the app:

kibana/x-pack/solutions/observability/plugins/observability_logs_explorer/public/state_machines/observability_logs_explorer/src/url_state_storage_service.ts

Lines 51 to 61 in 4396d3a

const urlStateValues =
urlStateStorageContainer.get<unknown>(OBSERVABILITY_LOGS_EXPLORER_URL_STATE_KEY) ?? undefined;
// in the future we'll have to more schema versions to the union
const stateValuesE = rt
.union([
rt.undefined,
urlSchemaV1.stateFromUntrustedUrlRT,
urlSchemaV2.stateFromUntrustedUrlRT,
])
.decode(urlStateValues);

What do you think?
.../solutions/observability/plugins/observability_logs_explorer/public/redirect_to_discover.tsx Outdated
Comment on lines 53 to 57
const gEncoded = encode(gState);
const aEncoded = encode(aState);

const newSearch = `#/?_g=${gEncoded}&_a=${aEncoded}`;
const path = `${location.pathname}${newSearch}`;
Copy link
Member

@weltenwort weltenwort Apr 10, 2025

Choose a reason for hiding this comment

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

Agreed - and although navigating like this directly in the component was already introduced in an earlier PR, I wonder if we shouldn't do this cleanly and use the proper locator in a proper effect. Does that make sense?
weltenwort
weltenwort reviewed Apr 24, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Nice, getting closer 🐎 Most columns now seem to carry over, but I still commented on some missing pieces below.

For simplicity I would suggest to resurrect the old code path that already existed to generate a link to Discover: https://github.com/elastic/kibana/blob/old-8.x/x-pack/solutions/observability/plugins/observability_logs_explorer/public/components/discover_link.tsx

Only instead of rendering a button we use it in an effect to redirect. Does that make sense?
...lutions/observability/plugins/observability_logs_explorer/public/logs_explorer_url_schema.ts Outdated
Comment on lines 66 to 70
export type DataSourceSelection =
| { selectionType: 'all' }
| { selectionType: 'dataView'; selection: { dataView: unknown } }
| { selectionType: 'single'; selection: SingleDatasetSelection }
| { selectionType: 'unresolved'; selection: UnresolvedDatasetSelection };
Copy link
Member

@weltenwort weltenwort Apr 24, 2025

Choose a reason for hiding this comment

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

I think we need to use this to derive the correct data view for Discover to use. Otherwise it will use the "default" data view, which can be anything.

If that helps, here is one of the old logs explorer URLs that I'm using for testing: /app/observability-logs-explorer/?pageState=(breakdownField:log.level,columns:!((field:agent.name,type:document-field,width:290),(field:message,type:document-field)),controls:(data_stream.namespace:(mode:include,selection:(selectedOptions:!(),type:options))),dataSourceSelection:(selectionType:all),filters:!((meta:(alias:!n,disabled:!f,field:agent.ephemeral_id,index:%27dataset-logs-*-*%27,key:agent.ephemeral_id,negate:!f,type:exists,value:exists),query:(exists:(field:agent.ephemeral_id)))),query:(language:kuery,query:retry),refreshInterval:(pause:!t,value:5000),rowHeight:0,rowsPerPage:100,time:(from:%272025-04-23T17:51:50.564Z%27,to:%272025-04-23T18:07:01.865Z%27),v:2)#/
.../solutions/observability/plugins/observability_logs_explorer/public/redirect_to_discover.tsx Outdated
filters: parsedState.filters,
query: parsedState.query,
breakdownField: parsedState.chart?.breakdownField ?? undefined,
columns: parsedState.grid?.columns?.map((column) => column.field),
Copy link
Member

@weltenwort weltenwort Apr 24, 2025

Choose a reason for hiding this comment

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

Does this account for the different column types? Not all have a single field if I remember correctly.
weltenwort
weltenwort reviewed Apr 30, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thanks for adding the translation from the data source to a data view. That looks 99% correct - I just left a few final questions below.

Ideally we'd have some tests for the url state translation. What do you think? Would unit tests make sense? Or would you prefer a functional test?
...tions/observability/plugins/observability_logs_explorer/public/logs_explorer_schema_types.ts Outdated
Comment on lines 131 to 152
export type {
Column,
DataSourceSelectionPlain,
DataViewSelectionPayload,
SingleDatasetSelectionPayload,
UnresolvedDatasetSelectionPayload,
DataViewSpecWithId,
BaseUrlSchema,
UrlSchemaV1,
UrlSchemaV2,
ControlsState,
FilterSelection,
ChartDisplayOptions,
GridDisplayOptions,
GridColumnDisplayOptions,
GridRowsDisplayOptions,
DisplayOptions,
OptionsListControlOption,
OptionsListControlExists,
OptionsListControl,
ControlOptions,
};
Copy link
Member

@weltenwort weltenwort Apr 30, 2025

Choose a reason for hiding this comment

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

Nit 😇: AFAIK in the Kibana codebase this pattern is usually only applied when re-exporting types in index.ts files. Otherwise we directly use export interface X {} and the like.
...lutions/observability/plugins/observability_logs_explorer/public/logs_explorer_url_schema.ts Outdated

export const hydrateDataSourceSelection = (
dataSourceSelection: DataSourceSelectionPlain,
allSelection: { selectionType: 'all' }
Copy link
Member

@weltenwort weltenwort Apr 30, 2025

Choose a reason for hiding this comment

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

Should this be used to check for the 'all' selection type?
...lutions/observability/plugins/observability_logs_explorer/public/logs_explorer_url_schema.ts
query: { exists: { field: key } },
});

export const getDiscoverFiltersFromState = (
Copy link
Member

@weltenwort weltenwort Apr 30, 2025

Choose a reason for hiding this comment

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

Neat, thanks for remembering the namespace controls!
@rStelmach
Copy link
Contributor Author

rStelmach commented May 2, 2025

Thanks for adding the translation from the data source to a data view. That looks 99% correct - I just left a few final questions below.

Ideally we'd have some tests for the url state translation. What do you think? Would unit tests make sense? Or would you prefer a functional test?

@weltenwort Thank you for your recommendations. I added some unit tests that I think will cover the functionality. Let me know if that would be enough :)
weltenwort
weltenwort reviewed May 8, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thanks for adding the thorough tests!
weltenwort
weltenwort approved these changes May 21, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort 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, looks really good to me! I especially appreciate the new unit tests 💚

I left just one last minor suggestion below.
@elasticmachine
Copy link
Contributor

elasticmachine commented May 22, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #11 / SuggestUsersPopover calls onUsersChange when 1 user is selected
  • [job] [logs] FTR Configs #101 / Synthetics API Tests SyncGlobalParams parsed params for previously added http monitors

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityLogsExplorer 17 81 +64

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.1MB 1.1MB +184.0B
observabilityLogsExplorer 817.0B 3.9KB +3.1KB
total +3.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observabilityLogsExplorer 4.2KB 4.4KB +185.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
observabilityLogsExplorer 7 6 -1

Total ESLint disabled count

id before after diff
observabilityLogsExplorer 7 6 -1

History
@rStelmach rStelmach merged commit b412be2 into elastic:main May 23, 2025
10 checks passed
@kibanamachine
Copy link
Contributor

kibanamachine commented May 23, 2025

Starting backport for target branches: 8.19

https://github.com/elastic/kibana/actions/runs/15205038176
@kibanamachine
Copy link
Contributor

kibanamachine commented May 23, 2025

💔 All backports failed

Status Branch Result
❌ 8.19 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 215867

Questions ?

Please refer to the Backport tool documentation
@rStelmach rStelmach mentioned this pull request May 23, 2025
Merged
@rStelmach
Copy link
Contributor Author

rStelmach commented May 23, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation
rStelmach added a commit to rStelmach/kibana that referenced this pull request May 23, 2025
@rStelmach
[One Discover] Pass app state and global state to locator when redire…
9855b63 
…cting from /stream path (elastic#215867)

Modified the /stream route and redirect handlers from Logs Explorer to
carry state parameters when redirecting between:

Logs Stream → Logs Explorer

Logs Stream → Discover

Logs Explorer → Discover

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit b412be2)

# Conflicts:
#	x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx
#	x-pack/solutions/observability/plugins/observability_logs_explorer/public/applications/redirect_to_observability_logs_explorer.tsx
#	x-pack/solutions/observability/plugins/observability_logs_explorer/public/plugin.ts
#	x-pack/solutions/observability/plugins/observability_logs_explorer/public/types.ts
#	x-pack/solutions/observability/plugins/observability_logs_explorer/tsconfig.json
@kibanamachine kibanamachine mentioned this pull request May 26, 2025
Merged
1 task
rStelmach added a commit that referenced this pull request May 26, 2025
@rStelmach @kibanamachine
[8.19] [One Discover] Pass app state and global state to locator when…
e9d3382 
… redirecting from /stream path (#215867) (#221354)

# Backport

This will backport the following commits from `main` to `8.19`:
- [[One Discover] Pass app state and global state to locator when
redirecting from /stream path
(#215867)](#215867)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Robert
Stelmach","email":"60304951+rStelmach@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-05-23T07:40:44Z","message":"[One
Discover] Pass app state and global state to locator when redirecting
from /stream path (#215867)\n\nModified the /stream route and redirect
handlers from Logs Explorer to\ncarry state parameters when redirecting
between:\n\nLogs Stream → Logs Explorer\n\nLogs Stream →
Discover\n\nLogs Explorer → Discover\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"b412be27530c01a96cec87af575fb1ccb7ac3b60","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Discover","release_note:fix","Team:obs-ux-logs","backport:version","v9.1.0","v8.19.0"],"title":"[One
Discover] Pass app state and global state to locator when redirecting
from /stream
path","number":215867,"url":"https://github.com/elastic/kibana/pull/215867","mergeCommit":{"message":"[One
Discover] Pass app state and global state to locator when redirecting
from /stream path (#215867)\n\nModified the /stream route and redirect
handlers from Logs Explorer to\ncarry state parameters when redirecting
between:\n\nLogs Stream → Logs Explorer\n\nLogs Stream →
Discover\n\nLogs Explorer → Discover\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"b412be27530c01a96cec87af575fb1ccb7ac3b60"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/215867","number":215867,"mergeCommit":{"message":"[One
Discover] Pass app state and global state to locator when redirecting
from /stream path (#215867)\n\nModified the /stream route and redirect
handlers from Logs Explorer to\ncarry state parameters when redirecting
between:\n\nLogs Stream → Logs Explorer\n\nLogs Stream →
Discover\n\nLogs Explorer → Discover\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"b412be27530c01a96cec87af575fb1ccb7ac3b60"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
@rStelmach @kibanamachine @akowalska622
[One Discover] Pass app state and global state to locator when redire…
e13a5e5 
…cting from /stream path (elastic#215867)

Modified the /stream route and redirect handlers from Logs Explorer to
carry state parameters when redirecting between:

Logs Stream → Logs Explorer

Logs Stream → Discover

Logs Explorer → Discover

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@thomheymann thomheymann mentioned this pull request Aug 13, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application release_note:fix Team:obs-onboarding Observability Onboarding Team v8.19.0 v9.1.0

5 participants

@rStelmach @elasticmachine @kibanamachine @weltenwort @davismcphee