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

Implement a generic solution to execution context propagation inside Kibana #132629

Closed
Tracked by #130971
xcrzx opened this issue May 20, 2022 · 14 comments
Closed
Tracked by #130971

Implement a generic solution to execution context propagation inside Kibana #132629

xcrzx opened this issue May 20, 2022 · 14 comments
Assignees
Labels
performance Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture

Comments

@xcrzx
Copy link
Contributor

xcrzx commented May 20, 2022

Summary

In #131805 we introduced a wrapper around the Route component from react-router-dom (see src/plugins/kibana_react/public/router/router.tsx). This wrapper is intended to be used in Kibana across all solutions for automatic propagation of execution context when a user changes route. To do that:

  1. We need to extract the route wrapper from the @kbn/kibana-react-plugin package to a separate single-purpose package, something like @kbn/shared-ux-router. This is yet to be discussed with @majagrubic and @clintandrewhall
  2. To make the transition easier, we want to re-export other react-router-dom exports alongside the modified' Route'.
  3. Solutions should start using that Route component in place of the one from react-router-dom. I.e. replace all occurrences of import { ... } from 'react-router-dom' with import { ... } from '@kbn/shared-ux-router'.
  4. All manual calls to useExecutionContext are not needed anymore and should be removed.
  5. After the transition is completed, we can introduce an ESlint rule that ensures that react-router-dom is not used directly in Kibana and that imports go through the new @kbn/shared-ux-router package.

For more context see this discussion: #131805 (comment)

@xcrzx xcrzx added performance technical debt Improvement of the software architecture and operational architecture 8.4 candidate labels May 20, 2022
@xcrzx xcrzx self-assigned this May 20, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label May 20, 2022
@xcrzx xcrzx added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label May 20, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@botelastic botelastic bot removed the needs-team Issues missing a team label label May 20, 2022
@xcrzx xcrzx added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label May 20, 2022
@lizozom
Copy link
Contributor

lizozom commented Jul 7, 2022

@xcrzx wanted to check in about the status of this :-)

@lizozom
Copy link
Contributor

lizozom commented Jul 20, 2022

As per @xcrzx working on this issue was postponed.
Moving back to backlog.

@rshen91 rshen91 changed the title Implement a generic solution to execution context propagation inside Kinbana Implement a generic solution to execution context propagation inside Kibana Jul 26, 2022
@lukeelmers
Copy link
Member

lukeelmers commented Nov 16, 2022

@rshen91 @xcrzx @lizozom Was this issue addressed in #138544, or is there more to do here?

(Edit: I guess we need each of the remaining apps to switch to using the shared router then?)

@xcrzx
Copy link
Contributor Author

xcrzx commented Nov 17, 2022

I guess we need each of the remaining apps to switch to using the shared router then?

Yes, that's correct. Once all apps are migrated to the shared router, we can close this issue.

@lukeelmers
Copy link
Member

Do we have anywhere we are tracking which apps still need to migrate? I'm happy to help chase people down if it helps push things forward :)

@xcrzx
Copy link
Contributor Author

xcrzx commented Nov 18, 2022

Do we have anywhere we are tracking which apps still need to migrate? I'm happy to help chase people down if it helps push things forward :)

I don't know of any ticket besides this where the progress is tracked. I believe only the security solution has migrated to the Route component with automatic execution context propagation. So all other apps are yet to follow.

Probably the easiest way to see what plugins need to be migrated is by making this ESLint rule global (currently, it is enabled only for security solution plugins) and collecting all errors:

kibana/.eslintrc.js

Lines 1007 to 1014 in 4de1ab5

paths: [
{
name: 'react-router-dom',
importNames: ['Route'],
message: "import { Route } from '@kbn/kibana-react-plugin/public'",
},
],
},

The migration itself should be pretty straightforward. Plugins need to replace Router imports from react-router-dom with imports from @kbn/kibana-react-plugin and remove manual calls to useExecutionContext if there are any.

@lukeelmers Please let me know how I could help move this effort forward.

@clintandrewhall
Copy link
Contributor

We could start by adding this eslint rule to a PR and see what blows up?

@xcrzx
Copy link
Contributor Author

xcrzx commented Nov 21, 2022

We could start by adding this eslint rule to a PR and see what blows up?

Created a draft PR: #145863.

@lukeelmers
Copy link
Member

Plugins need to replace Router imports from react-router-dom with imports from @kbn/kibana-react-plugin and remove manual calls to useExecutionContext if there are any

I'm seeing ~15 plugins that are calling useExecutionContext, and all of the rest of the changes would be simply updating import statements. If this is all that needs to happen, I wonder if it would be worth prioritizing this on the Kibana side instead so we can call it done. Getting the individual codeowners of these plugins to make changes on their end could take months. WDYT @clintandrewhall?

The reason I'm interested is because we have some large-scale users who have been asking for Kibana-wide execution context propagation, and this is the last thing preventing us from being able to say that it is officially supported everywhere.

@lukeelmers
Copy link
Member

@elastic/kibana-global-experience Just wanted to bump this for y'all to consider when prioritizing upcoming work. Seems like it could be a relatively easy win and a big benefit for some of our largest deployments.

mistic added a commit that referenced this issue Feb 14, 2023
## Summary

This PR removes all imports of Route from react-router-dom and
'@kbn/kibana-react-plugin/public' and instead imports Route from
@kbn/shared-ux-router.

### Context
Based on
#132629 (comment) This PR
executes steps 2 - 4:

> 2. To make the transition easier, we want to re-export other
react-router-dom exports alongside the modified' Route'.
> 3. Solutions should start using that Route component in place of the
one from react-router-dom. I.e. replace all occurrences of import { ...
} from 'react-router-dom' with import { ... } from
'@kbn/shared-ux-router'.
> 4. All manual calls to useExecutionContext are not needed anymore and
should be removed.

### Future PR

Looks like this might be getting worked on in:
#145863 (thanks!)

> Introduce an ESlint rule that ensures that react-router-dom is not
used directly in Kibana and that imports go through the new
@kbn/shared-ux-router package.

This is tangentially accomplished through
#150340 but only addresses using
Route through @kbn/kibana-react-plugin/public'


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
justinkambic pushed a commit to justinkambic/kibana that referenced this issue Feb 23, 2023
## Summary

This PR removes all imports of Route from react-router-dom and
'@kbn/kibana-react-plugin/public' and instead imports Route from
@kbn/shared-ux-router.

### Context
Based on
elastic#132629 (comment) This PR
executes steps 2 - 4:

> 2. To make the transition easier, we want to re-export other
react-router-dom exports alongside the modified' Route'.
> 3. Solutions should start using that Route component in place of the
one from react-router-dom. I.e. replace all occurrences of import { ...
} from 'react-router-dom' with import { ... } from
'@kbn/shared-ux-router'.
> 4. All manual calls to useExecutionContext are not needed anymore and
should be removed.

### Future PR

Looks like this might be getting worked on in:
elastic#145863 (thanks!)

> Introduce an ESlint rule that ensures that react-router-dom is not
used directly in Kibana and that imports go through the new
@kbn/shared-ux-router package.

This is tangentially accomplished through
elastic#150340 but only addresses using
Route through @kbn/kibana-react-plugin/public'


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
@rudolf
Copy link
Contributor

rudolf commented Mar 15, 2023

@vadimkibana My understanding is that the scope of this work is relatively small. But there is enormous value in having richer execution context across the whole UI. This would allow both us and users to better attribute where load on Kibana/Elasticsearch is originating from which is a highly requested feature.

Is there a reason this work has been cancelled?

@lukeelmers
Copy link
Member

I was under the impression that #150357 addressed the full scope of this issue, but I may be mistaken.

@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 16, 2023

This ticket was fully addressed in #150357, so closing it as completed

@xcrzx xcrzx closed this as completed Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

8 participants