-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Execution context remains the same on every route change #195778
Comments
Pinging @elastic/appex-sharedux (Team:SharedUX) |
My guess is that this got broken when we moved the sharedux router to packages (a while ago). We probably lost context that was picked up from kibana_react. The fix should probably be to add this context to KibanaRenderContextProvider and use it from there. This only be fixed for |
I took a look, I believe this is the context, hope I got it correctly:
So atm and for quite a while, I believe, only the places where we explicitly call |
…eExecutionContextTracking flag (#204547) Resolves #195778 ## 🐞 Summary This PR fixes missing executionContext in sharedux router by adding `SharedUXContext` to the `KibanaRootContextProvider` (inside of the `KibanaRenderContextProvider`). (More context provided in this #195778 (comment)) It also introduces `enableExecutionContextTracking` to enable tracking logic to avoid creating a race condition for the existing custom execution context tracking implementations. I enabled this flag for the observability plugin and here is the difference: |Item|Screenshot| |---|---| |Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)| |After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)| ### 🧪 How to test - Go to the observability alerts page and check the kibana-browser request as shown above ### ✨ Possible future improvements Allowing this logic to be provided by the consumer so that we can get rid of custom implementations (Example: [APM custom execution context](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25)) --------- Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: Davis McPhee <[email protected]> Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: Elena Stoeva <[email protected]>
…eExecutionContextTracking flag (elastic#204547) Resolves elastic#195778 ## 🐞 Summary This PR fixes missing executionContext in sharedux router by adding `SharedUXContext` to the `KibanaRootContextProvider` (inside of the `KibanaRenderContextProvider`). (More context provided in this elastic#195778 (comment)) It also introduces `enableExecutionContextTracking` to enable tracking logic to avoid creating a race condition for the existing custom execution context tracking implementations. I enabled this flag for the observability plugin and here is the difference: |Item|Screenshot| |---|---| |Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)| |After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)| ### 🧪 How to test - Go to the observability alerts page and check the kibana-browser request as shown above ### ✨ Possible future improvements Allowing this logic to be provided by the consumer so that we can get rid of custom implementations (Example: [APM custom execution context](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25)) --------- Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: Davis McPhee <[email protected]> Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: Elena Stoeva <[email protected]> (cherry picked from commit 53748fd) # Conflicts: # packages/react/kibana_context/root/BUILD.bazel
…eExecutionContextTracking flag (elastic#204547) Resolves elastic#195778 ## 🐞 Summary This PR fixes missing executionContext in sharedux router by adding `SharedUXContext` to the `KibanaRootContextProvider` (inside of the `KibanaRenderContextProvider`). (More context provided in this elastic#195778 (comment)) It also introduces `enableExecutionContextTracking` to enable tracking logic to avoid creating a race condition for the existing custom execution context tracking implementations. I enabled this flag for the observability plugin and here is the difference: |Item|Screenshot| |---|---| |Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)| |After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)| ### 🧪 How to test - Go to the observability alerts page and check the kibana-browser request as shown above ### ✨ Possible future improvements Allowing this logic to be provided by the consumer so that we can get rid of custom implementations (Example: [APM custom execution context](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25)) --------- Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: Davis McPhee <[email protected]> Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: Elena Stoeva <[email protected]> (cherry picked from commit 53748fd) # Conflicts: # packages/react/kibana_context/render/render_provider.tsx # packages/react/kibana_context/root/BUILD.bazel # packages/react/kibana_context/root/root_provider.test.tsx # packages/react/kibana_context/root/root_provider.tsx # packages/react/kibana_context/root/tsconfig.json
… enableExecutionContextTracking flag (#204547) (#204798) # Backport This will backport the following commits from `main` to `8.x`: - [Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag (#204547)](#204547) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Maryam Saeidi","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-18T12:59:23Z","message":"Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag (#204547)\n\nResolves https://github.com/elastic/kibana/issues/195778\r\n\r\n## 🐞 Summary\r\nThis PR fixes missing executionContext in sharedux router by adding\r\n`SharedUXContext` to the `KibanaRootContextProvider` (inside of the\r\n`KibanaRenderContextProvider`). (More context provided in this\r\nhttps://github.com//issues/195778#issuecomment-2426936142)\r\n\r\nIt also introduces `enableExecutionContextTracking` to enable tracking\r\nlogic to avoid creating a race condition for the existing custom\r\nexecution context tracking implementations.\r\n\r\nI enabled this flag for the observability plugin and here is the\r\ndifference:\r\n\r\n|Item|Screenshot|\r\n|---|---|\r\n\r\n|Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)|\r\n\r\n|After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)|\r\n\r\n### 🧪 How to test\r\n- Go to the observability alerts page and check the kibana-browser\r\nrequest as shown above\r\n\r\n### ✨ Possible future improvements\r\n\r\nAllowing this logic to be provided by the consumer so that we can get\r\nrid of custom implementations (Example: [APM custom execution\r\ncontext](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25))\r\n\r\n---------\r\n\r\nCo-authored-by: Anton Dosov <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: Elena Stoeva <[email protected]>","sha":"53748fdefa1d59d58a4708258a1476dc140b1588","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Team:obs-ux-management"],"number":204547,"url":"https://github.com/elastic/kibana/pull/204547","mergeCommit":{"message":"Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag (#204547)\n\nResolves https://github.com/elastic/kibana/issues/195778\r\n\r\n## 🐞 Summary\r\nThis PR fixes missing executionContext in sharedux router by adding\r\n`SharedUXContext` to the `KibanaRootContextProvider` (inside of the\r\n`KibanaRenderContextProvider`). (More context provided in this\r\nhttps://github.com//issues/195778#issuecomment-2426936142)\r\n\r\nIt also introduces `enableExecutionContextTracking` to enable tracking\r\nlogic to avoid creating a race condition for the existing custom\r\nexecution context tracking implementations.\r\n\r\nI enabled this flag for the observability plugin and here is the\r\ndifference:\r\n\r\n|Item|Screenshot|\r\n|---|---|\r\n\r\n|Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)|\r\n\r\n|After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)|\r\n\r\n### 🧪 How to test\r\n- Go to the observability alerts page and check the kibana-browser\r\nrequest as shown above\r\n\r\n### ✨ Possible future improvements\r\n\r\nAllowing this logic to be provided by the consumer so that we can get\r\nrid of custom implementations (Example: [APM custom execution\r\ncontext](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25))\r\n\r\n---------\r\n\r\nCo-authored-by: Anton Dosov <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: Elena Stoeva <[email protected]>","sha":"53748fdefa1d59d58a4708258a1476dc140b1588"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/204547","number":204547,"mergeCommit":{"message":"Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag (#204547)\n\nResolves https://github.com/elastic/kibana/issues/195778\r\n\r\n## 🐞 Summary\r\nThis PR fixes missing executionContext in sharedux router by adding\r\n`SharedUXContext` to the `KibanaRootContextProvider` (inside of the\r\n`KibanaRenderContextProvider`). (More context provided in this\r\nhttps://github.com//issues/195778#issuecomment-2426936142)\r\n\r\nIt also introduces `enableExecutionContextTracking` to enable tracking\r\nlogic to avoid creating a race condition for the existing custom\r\nexecution context tracking implementations.\r\n\r\nI enabled this flag for the observability plugin and here is the\r\ndifference:\r\n\r\n|Item|Screenshot|\r\n|---|---|\r\n\r\n|Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)|\r\n\r\n|After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)|\r\n\r\n### 🧪 How to test\r\n- Go to the observability alerts page and check the kibana-browser\r\nrequest as shown above\r\n\r\n### ✨ Possible future improvements\r\n\r\nAllowing this logic to be provided by the consumer so that we can get\r\nrid of custom implementations (Example: [APM custom execution\r\ncontext](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25))\r\n\r\n---------\r\n\r\nCo-authored-by: Anton Dosov <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: Elena Stoeva <[email protected]>","sha":"53748fdefa1d59d58a4708258a1476dc140b1588"}}]}] BACKPORT-->
…eExecutionContextTracking flag (elastic#204547) Resolves elastic#195778 ## 🐞 Summary This PR fixes missing executionContext in sharedux router by adding `SharedUXContext` to the `KibanaRootContextProvider` (inside of the `KibanaRenderContextProvider`). (More context provided in this elastic#195778 (comment)) It also introduces `enableExecutionContextTracking` to enable tracking logic to avoid creating a race condition for the existing custom execution context tracking implementations. I enabled this flag for the observability plugin and here is the difference: |Item|Screenshot| |---|---| |Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)| |After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)| ### 🧪 How to test - Go to the observability alerts page and check the kibana-browser request as shown above ### ✨ Possible future improvements Allowing this logic to be provided by the consumer so that we can get rid of custom implementations (Example: [APM custom execution context](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25)) --------- Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: Davis McPhee <[email protected]> Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: Elena Stoeva <[email protected]>
Kibana version:
main (8.15)
Describe the bug:
This issue was reported by on slack, it seems the pageName in the execution context remains the same on every route change. Ideally the on route change the we should set the values of the execution, this typically includes
pageName
, alongside couple of other values. On investigating this it seems the implementation to gather the details for the current page's context does exist, however it doesn't get persisted to the execution context instance because it's also undefined within said implementation, hence why subscribers receive a stale valueSteps to reproduce:
Expected behavior:
The text was updated successfully, but these errors were encountered: