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

Fix context.pageName by fixing missing executionContext #201118

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

maryam-saeidi
Copy link
Member

@maryam-saeidi maryam-saeidi commented Nov 21, 2024

Resolves #195778

🐞 Summary

This PR fixes missing executionContext in sharedux router by adding SharedUXContext to the KibanaRenderContextProvider. (More context provided in this comment)

After this change, we will have the route in the page name as well:

image

⚠️ Note for reviewer

To ensure this logic does not have a negative impact on custom implementation in plugins and also possibly remove duplicate implementation that can be covered with this implementation, I added a temporary TODO comment to different plugins that have a custom implementation. I would appreciate it if someone from the team that owns those plugins would take a look at the usage of useExecutionContext and ensure this logic works as expected after merging this PR according to the "How to test" section.

It is worth mentioning that the APM customized execution context had worked as expected when I tested it with a static value.

APM test

image

🧪 How to test

This is the shared implementation that is provided by KibanaRenderContextProvider

useSharedUXExecutionContext(executionContext, {
    type: 'application',
    page: match.path,
    id: Object.keys(match.params).length > 0 ? JSON.stringify(match.params) : undefined,
  });

This logic will be visible in the kibana-browser request on your page as shown below: (here is the code for setting pageName)

image

For your plugin, please check the following:

  • For the instances of useExecutionContext
    • if this matches the above implementation, please check if everything works as expected if you remove the custom useExecutionContext usage.
      • If yes, please do so and commit the change to this PR.
      • If not, does the current implementation affect your custom implementation? Please let me know if that's the case.
  • After checking this logic, please approve this PR and add a commit to this PR to remove // TODO: Remove this comment after testing.

Plugin overview

Plugin Works with shared logic Comment
discover comment
data_quality comment
dashboard comment
cross_cluster_replication comment
index_management comment
rollup comment
snapshot_restore comment
upgrade_assistant comment
security_solution comment

@maryam-saeidi maryam-saeidi added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 21, 2024
@maryam-saeidi maryam-saeidi self-assigned this Nov 21, 2024
@maryam-saeidi
Copy link
Member Author

Hi @kpatticha ,

I checked this implementation for APM pages to ensure this does not interfere with APM custom implementation, and it looks fine to me:

image

Was APM the only application with a custom logic? Do you mind testing this PR and verify if the previous custom implementations still have the same context.pageName?

@Dosant
Copy link
Contributor

Dosant commented Nov 22, 2024

Was APM the only application with a custom logic? Do you mind testing this PR and verify if the previous custom implementations still have the same context.pageName?

@maryam-saeidi, if I understand correctly it is any places where useExecutionContext is called directly:

https://github.com/search?q=repo%3Aelastic%2Fkibana+useExecutionContext%28&type=code

Or executionContext.set

https://github.com/search?q=repo%3Aelastic%2Fkibana+executionContext.set%28&type=code

@kpatticha
Copy link
Contributor

Hi @kpatticha ,

I checked this implementation for APM pages to ensure this does not interfere with APM custom implementation, and it looks fine to me:

image

Was APM the only application with a custom logic? Do you mind testing this PR and verify if the previous custom implementations still have the same context.pageName?

Tested APM and it works as expected 💯 I've noticed the following in security timeline page. The localhost seems correct to me but it might worth checking or inform the team that the fix in this PR could potentially impact their telemetry data

Screenshot 2024-11-22 at 13 55 27

Also, @afharo opened a PR trying to address the same problem #200785 .Have you had a chance to review it? It might be worth coordinating to avoid duplicating efforts. 😊

Comment on lines +30 to +31
/** The `ExecutionContextStart` API from `CoreStart`. */
executionContext?: ExecutionContextStart;
Copy link
Member

Choose a reason for hiding this comment

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

Q: to avoid errors in the future, should we make this one mandatory?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check to see if it is OK to make this dependency mandatory.
Still, I don't think this will prevent similar issues in the future because, eventually, it will be passed to SharedUXContext, which is initialized with an empty executionContext here.

@maryam-saeidi maryam-saeidi marked this pull request as ready for review November 25, 2024 10:22
@maryam-saeidi maryam-saeidi requested review from a team as code owners November 25, 2024 10:22
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Nov 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@maryam-saeidi
Copy link
Member Author

Also, @afharo opened a PR trying to address the same problem #200785 .Have you had a chance to review it?

@kpatticha Yes, I checked that PR, and I think since we already have SharedUXContext that can be included inrootContextProvider, fixing it this way makes more sense. In the current implementation of this PR, we also have the possibility of disabling this logic for plugins if they want to continue with the custom implementation or if the shared logic is causing an issue (we just need to make rootContextProvider configurable).

@@ -16,6 +16,7 @@ export const useUpdateExecutionContext = () => {
useEffect(() => {
// setImmediate is required to ensure that EBT telemetry for the previous page is shipped before the new page is updated
setImmediate(() => {
// TODO: Remove this comment after testing
executionContext.set({ page: pathname });
Copy link
Member Author

@maryam-saeidi maryam-saeidi Nov 25, 2024

Choose a reason for hiding this comment

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

@kpatticha noticed a difference in the new context:

image

@elastic/security-solution Any idea how we can solve this issue?
In APM customized execution context, overriding the value is working as expected with the changes in this PR. I wonder what the difference between these two cases is.

Copy link
Contributor

@Dosant Dosant Nov 28, 2024

Choose a reason for hiding this comment

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

It seems like the same problem as with the data_quality page. The custom hook is called before the hook in our Route

Copy link
Contributor

@Dosant Dosant Nov 28, 2024

Choose a reason for hiding this comment

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

And there is slight implementation difference which is probably why the pageName is different

we use : const match = useRouteMatch(); match.path
security uses: const { pathname } = useLocation();

@afharo
Copy link
Member

afharo commented Nov 25, 2024

Also, @afharo opened a PR trying to address the same problem #200785 .Have you had a chance to review it?

@kpatticha Yes, I checked that PR, and I think since we already have SharedUXContext that can be included inrootContextProvider, fixing it this way makes more sense. In the current implementation of this PR, we also have the possibility of disabling this logic for plugins if they want to continue with the custom implementation or if the shared logic is causing an issue (we just need to make rootContextProvider configurable).

++! My PR was just exploration work... but this solution looks like a better approach

@ElenaStoeva ElenaStoeva self-requested a review November 25, 2024 16:30
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.

Reviewed Data Discovery usages, LGTM 👍

@@ -102,6 +102,7 @@ export const ContextApp = ({ dataView, anchorId, referrer }: ContextAppProps) =>
});
}, [locator, referrer, services]);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I reviewed our usages in Discover and dropping the custom overrides changes the context properties. E.g. for this one specifically:

page: "context" -> page: "/context/:dataViewId/:id"
entityId: "90943e30-9a47-11e8-b64d-95841ca0b247" -> entityId: "{\"dataViewId\":\"90943e30-9a47-11e8-b64d-95841ca0b247\",\"id\":\"Am8GZZMBvuc8nvvEfgGJ\"}"

I kept the overrides and removed the TODO here: 89b004b.

@@ -44,7 +44,6 @@ const AppWithExecutionContext = ({
}) => {
const { executionContext } = core;

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

@maryam-saeidi this override is on purpose as described in
[Dataset quality] Adding execution context to data quality page within management app.

I removed the comment with fix(dataset-quality): remove TODO

However, it seems your changes are affecting how the pageName is sent for this app.

Before this PR, the pageName for dataset quality was constantly tracked as application:management:data_quality.

Screenshot 2024-11-26 at 12 56 21

Visiting the same page on this PR, we get relative paths for the sub-router of this app instead of the target app.

Screenshot 2024-11-26 at 12 57 11

Can't say exactly what is the turning point, but the values are different now 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

@maryam-saeidi, I think the problem here is that manual useExecutionContext() is called before the ones that are called in Routes, so Route overrides the custom one. Ideally the final one could look like this: "application: management: data_quality:details"

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

APM and Infra look good. Profiling is not manually changing the execution context on route changes, as APM does it , so the context pageName never changes. Are going to address that in a separate PR?

@maryam-saeidi
Copy link
Member Author

APM and Infra look good. Profiling is not manually changing the execution context on route changes, as APM does it , so the context pageName never changes. Are going to address that in a separate PR?

Thanks for checking, Carlos. This is not part of this PR, and it is up to the teams at the moment. We can discuss this with appex-sharedux to improve this logic, but it would be outside of the scope of this PR.

@@ -30,6 +30,7 @@ export const DashboardListingTable = ({
urlStateEnabled,
showCreateDashboardButton = true,
}: DashboardListingProps) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

dashboard changes LGTM, comment needs to be removed

@darnautov darnautov self-requested a review November 28, 2024 12:58
Copy link
Contributor

@ElenaStoeva ElenaStoeva 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 working on this! Tested locally and some of the Kibana Management plugins have a different pageName context now.

@@ -34,6 +34,7 @@ const AppWithExecutionContext = ({
getUrlForApp: ApplicationStart['getUrlForApp'];
executionContext: ExecutionContextStart;
}) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a difference here:

Screenshot 2024-11-28 at 13 19 31

@@ -19,6 +19,7 @@ export const IndexList: React.FunctionComponent<RouteComponentProps> = ({ histor
core: { executionContext },
} = useAppContext();

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

@@ -30,6 +30,7 @@ const AppWithExecutionContext = ({
history: ManagementAppMountParams['history'];
executionContext: ExecutionContextStart;
}) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a difference here:

Screenshot 2024-11-28 at 13 23 38

@@ -87,6 +87,7 @@ export const PolicyList: React.FunctionComponent<RouteComponentProps<MatchParams
uiMetricService.trackUiMetric(UIM_POLICY_LIST_LOAD);
}, [uiMetricService]);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

@@ -160,6 +160,7 @@ export const RootComponent = (dependencies: AppDependencies) => {
core: { application, http, executionContext, ...startServices },
} = dependencies.services;

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

LGTM.

@elasticmachine
Copy link
Contributor

⏳ Build in-progress

  • Buildkite Build
  • Commit: 2a5a780
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-201118-2a5a780b0e0c

History

cc @maryam-saeidi

@@ -165,6 +165,7 @@ export function App({
setIndicateNoData(true);
}, [setIndicateNoData]);

// TODO: Remove this comment after testing
Copy link
Contributor

@dej611 dej611 Nov 29, 2024

Choose a reason for hiding this comment

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

Not sure it is strictly related to this PR, but from my quick testing I see these page names from different flows:

  • Create visualization in dashboard, opens a Lens editor => application:lens:editor
  • Create ES|QL panel in dashboard => application:dashboards:app
  • Edit inline in a Dashboard => application:dashboard:app
  • Edit in editor from a Dashboard inline editing panel => application:lens:editor
  • Edit visualization in Discover => application:lens:/edit_by_value

@@ -68,6 +68,7 @@ export const VisualizeEditor = ({ onAppLeave }: VisualizeAppProps) => {
);

const editorName = savedVisInstance?.vis.type.title.toLowerCase().replace(' ', '_') || '';
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I've checked this and noticed some subtle different pageName sometimes.
Nothing major, just these few issues:

  • application:visualize and application:visualize:editor are used for similar flows
  • application:visualize:editor:xxxxx is triggered only on navigating back to dashboard from editor, not on enter

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

I've tested various flows for all the editors (Lens & legacy): the events are triggered correctly, not sure about their content as described in the inline comments, but at least they are there.
While it would be nice to have more context on my comment, I'll approve the PR to unblock it.

@darnautov
Copy link
Contributor

@maryam-saeidi We also use execution context for embeddables here, which doesn't seem to pass a child attribute.

Example of the embeddable context on the dashboard app
{
  "timestamp": "2024-12-03T16:31:19.632Z",
  "event_type": "click",
  "context": {
    "isDev": true,
    "isDistributable": false,
    "version": "9.0.0",
    "branch": "main",
    "buildNum": 9007199254740991,
    "buildSha": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
    "session_id": "e5b78d05-c775-4c58-b5c9-6e028e27fe8b",
    "user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36",
    "preferred_language": "en-GB",
    "preferred_languages": [
      "en-GB",
      "en-US",
      "en",
      "ru",
      "nl"
    ],
    "viewport_width": 2511,
    "viewport_height": 695,
    "cluster_name": "default-9.0.0-main-1043",
    "cluster_uuid": "zhJo9MR_Rbeh1w92AcwuZQ",
    "cluster_version": "9.0.0-SNAPSHOT",
    "cluster_build_flavor": "default",
    "pageName": "dashboard:dashboards:app",
    "applicationId": "dashboards",
    "page": "app",
    "entityId": "b702185d-5512-4597-b0dc-195e94c671ad",
    "page_title": "ggg - Elastic",
    "page_url": "/app/dashboards#/view/b702185d-5512-4597-b0dc-195e94c671ad?_g=(filters:!(),refreshInterval:(pause:!f,value:30000),time:(from:'2024-11-10T00:39:02.912Z',to:'2024-12-03T16:15:00.000Z'))",
    "license_id": "66d90a0e-6eed-45fc-a2a4-08a9d21e60b3",
    "license_status": "active",
    "license_type": "trial",
    "labels": {},
    "discoverProfiles": [],
    "userId": "986051385feae5b9850804db2d701c0b029ad24f09bce340c12aee7a5c8a0391",
    "isElasticCloudUser": false
  },
  "properties": {
    "target": [
      "HTML",
      "lang=en",
      "BODY",
      "class=coreSystemRootDomElement euiBody--headerIsFixed kbnBody kbnBody--noHeaderBanner kbnBody--chromeVisible kbnVersion-9-0-0",
      "DIV",
      "id=kibana-body",
      "data-test-subj=kibanaChrome",
      "DIV",
      "class=kbnAppWrapper",
      "data-test-subj=kbnAppWrapper visibleChrome",
      "DIV",
      "class=kbnAppWrapper",
      "aria-busy=false",
      "DIV",
      "class=dashboardViewport",
      "DIV",
      "class=dashboardContainer",
      "DIV",
      "class=dshDashboardViewportWrapper",
      "DIV",
      "class=dshDashboardViewport",
      "data-shared-items-container=true",
      "data-title=ggg",
      "data-shared-items-count=1",
      "DIV",
      "class=react-grid-layout dshLayout--editing",
      "style=height: 428px;",
      "DIV",
      "class=dshDashboardGrid__item react-grid-item react-draggable react-resizable css-13nuqur-dashboard_grid_item--focusStyles",
      "data-test-subj=dashboardPanel",
      "id=panel-89ef8154-7ee0-46e0-8c17-2a40ccf249a4",
      "data-grid=[object Object]",
      "style=top: 8px; left: 0.318598%; width: 49.542%; height: 412px; position: absolute;",
      "DIV",
      "class=embPanel__hoverActionsAnchor css-1k23tud-presentation_panel_hover_actions--PresentationPanelHoverActions",
      "data-test-subj=embeddablePanelHoverActions-MLanomalyswimlaneforcatrogy_example",
      "data-test-embeddable-id=89ef8154-7ee0-46e0-8c17-2a40ccf249a4",
      "DIV",
      "class=euiPanel euiPanel--plain embPanel embPanel--editing css-szlwjt-euiPanel-grow-m-plain",
      "role=figure",
      "aria-labelledby=i00652701-b194-11ef-a80f-9f14aa095f80",
      "data-test-subj=embeddablePanel",
      "data-render-complete=true",
      "DIV",
      "class=embPanel__content",
      "DIV",
      "data-test-subj=mlAnomalySwimlaneEmbeddableWrapper",
      "data-shared-item=",
      "class=css-1osklss-anomaly_swimlane_embeddable_factory--Component",
      "DIV",
      "data-test-subj=mlSwimLaneEmbeddable_89ef8154-7ee0-46e0-8c17-2a40ccf249a4",
      "class=euiFlexGroup css-12hn1m1-euiFlexGroup-none-flexStart-stretch-column-swimlane_container--SwimlaneContainer",
      "DIV",
      "class=euiFlexItem css-18qej2d-euiFlexItem-growZero-swimlane_container--SwimlaneContainer",
      "DIV",
      "DIV",
      "class=css-1h3qr3u-swimlane_container--SwimlaneContainer",
      "style=height: 154px;",
      "DIV",
      "class=echChart",
      "style=width: 100%; height: 100%;",
      "DIV",
      "class=echChartContent mlSwimLaneContainer echChartContent--column",
      "DIV",
      "class=echContainer",
      "DIV",
      "class=echChartPointerContainer",
      "style=cursor: pointer;",
      "FIGURE",
      "aria-describedby=e71a3f69-ae2b-4e0d-856e-677abf63217f--defaultSummary",
      "CANVAS",
      "class=echCanvasRenderer",
      "width=2456",
      "height=240",
      "role=presentation",
      "style=width: 1228px; height: 120px;"
    ]
  }
}

@@ -98,6 +98,7 @@ export const useActiveRoute = (routesList: MlRoute[]): MlRoute => {
[activeRoute, overlays, pathname, startServices]
);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested, works as expected

Suggested change
// TODO: Remove this comment after testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution context remains the same on every route change