-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] Propagate execution context #131805
Conversation
3beb39f
to
111841c
Compare
...ecurity_solution/public/common/components/execution_context/execution_context_propagator.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/kibana-core (Team:Core) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can't go one step further and get normalized paths into core / the default execution context. This feels like something all teams can benefit from.
...ecurity_solution/public/common/components/execution_context/execution_context_propagator.tsx
Outdated
Show resolved
Hide resolved
|
||
useExecutionContext(executionContext, { | ||
type: 'application', | ||
page: normalizedPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it too, and would love to have it be integrated into Core, but given 1. that our routing system is agnostic and don't know about the underlying app's routing system or structure, and 2. the code that was added in the PR in normalizePath
, I'm not sure how we could make this generic/automatic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I was also thinking about a more generic solution. Probably, it would require wrapping the Route
component from react-routing-dom
and exporting it from Core. Something like this:
import { useExecutionContext } from '@kbn/kibana-react-plugin/public';
import { Route, useRouteMatch } from 'react-router-dom';
export const WrappedRoute = ({ children, ...props }) => {
return (
<Route {...props}>
<MatchPropagator />
{children}
</Route>
);
};
const MatchPropagator = () => {
const match = useRouteMatch();
useExecutionContext({
page: match.path,
});
return null;
};
That way, all solutions that use WrappedRoute
in place of Route
will have router paths automatically propagated. But there could be issues with that approach:
- All solutions will be tied to
react-router
. But that is probably fine, as I don't see any other routers in our repository. - It is unclear what path will be propagated if there are many matches on a page. The one from the last rendered
Route
? But that is probably worth exploring and building a PoC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear what path will be propagated if there are many matches on a page. The one from the last rendered Route? But that is probably worth exploring and building a PoC.
It's definitely worth exploring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, it would require wrapping the Route component from react-routing-dom and exporting it from Core
Core is meant to be agnostic, we won't expose anything react-related from it. But it can be exposed from the kbn-react
package/plugin the same way.
That aside, I really like the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @elastic/shared-ux for input - they own the kibana_react
plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of re-exporting library exports from plugins/packages, but I see the value here for consistency (importing everything router-related from the same location) and for future-proofing (e.g if we need to also modify the Router
component at some point), so I think I'm fine with it. But as Luke said, let's see what @elastic/shared-ux thinks about it
I also agree that we do want to move those things to their dedicated package at some point (which is atm not possible given the custom Route component requires types from kibana_react/Core)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx for tagging us. We're in the process of migrating to a package architecture and deprecating kibana_react
. So creating a separate package is definitely a way to go, and I would suggest doing it sooner rather than later. Please note though that we have a naming convention in place, so it would need to be nested under shared-ux
. I also don't like re-exporting exports from other packages; since we're moving to a package-oriented architecture, plugins will be importing things from multiple packages, I don't really see a problem there. Actually, I think the alternative is even worse: creating two of the same, so some consumers will import from react-router
, the others from our package.
Happy to chat offline further if you need help getting started on a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@majagrubic the new Route component relies on the useKibana
hook and on the executionContext types from Core, moving it to a package now seems difficult? (see https://github.com/elastic/kibana/pull/131805/files#diff-dcbcc6e829e4d48eb4efd34901aea43227549d42e62b1e5fbeb6eb24fc8ed1d3 and https://github.com/elastic/kibana/pull/131805/files#diff-e3c7f3a09f89cd016ae74b17148a26d9414d6dc3f18c78ee18fb2960f3247899)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo, we've synced with @majagrubic and agreed that extracting the new Route component to a package is out of the scope of this PR. Especially considering that we want the changes that affect Security Solution to be merged before FF (May 24), reducing the scope of changes would make sense. So in the first iteration, we're exporting only Route
from @kbn/kibana-react-plugin/public
and replacing Route components only in Security Solution and related plugins.
After FF, we will extract the Route component with other React router stuff to a separate package, as discussed above, and use it inside all other solutions.
UPD: created a task where the rest of work could be tracked #132629
Chatted with @xcrzx today about this approach: The final decidion should be made with the @elastic/kibana-core team, but personally I'm in favor of this approach, especially if url params get JSON.stringified into the If we do end up doing this, lets remove the existing instances of Thanks @xcrzx for taking this on! |
111841c
to
b6e9eec
Compare
ab6152a
to
5f042b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana_react
changes look good. Two minor questions/comments and then it's good to go
@@ -15,14 +15,14 @@ import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect'; | |||
* @param context | |||
*/ | |||
export function useExecutionContext( | |||
executionContext: CoreStart['executionContext'], | |||
executionContext: CoreStart['executionContext'] | undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the executionContext
type returned from useKibana().services
, it's ExecutionContextSetup | undefined
. I believe it definitely could be undefined in some cases, so it's better to handle that.
5f042b4
to
f474ac9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core changes lgtm
1bdd74f
to
3cf8861
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rules detections area changes LGTM
3cf8861
to
d447255
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared-ux
changes look good. code review only, haven't tested it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerts Area changes LGTM
d447255
to
cc84a20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for ResponseOps changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security-threat-hunting-explore changes LGTM
💛 Build succeeded, but was flakyFailed CI StepsTest FailuresMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: cc @xcrzx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Resolves: #126799
Related to: #129324
Summary
Utilized useExecutionContext in Security Solution to ensure all outgoing requests (search, saved objects) are correctly traced back to a page and an entity (in ES, APM, and Fullstory). Security Solution transactions are now properly labelled in APM:
Added more descriptive names to the
route-change
transactions, now they contain destination page path, likesecuritySolutionUI /hosts/:host/anomalies
:Previously those transactions looked like this:
How to test this PR
See the #124996 to make sure data is sent properly to APM.