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.

Sign up for GitHub

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

Jump to bottom

[APM] Sets up APM with new shared Kibana core context #43920

Merged
merged 4 commits into from
Aug 27, 2019
Merged

[APM] Sets up APM with new shared Kibana core context #43920

merged 4 commits into from
Aug 27, 2019

Conversation

jasonrhodes
Copy link
Member

Summary

In order to begin sharing utilities and components that rely on Kibana core functionality, we need to share a Kibana context provider that should wrap all of our apps. This PR swaps out the existing one in APM for a shared one that comes from the observability plugin so that it matches the same interface. This way we can share link utilities so that the link hrefs can be shared from a central place, with access to base path utilities, uiCapabilities, and other core utilities.

I'd like to get this in before moving onto the link work so that this doesn't become a merge nightmare with APM files.

If this is looking good, I can remove the APM core context and hook in this PR too.

@jasonrhodes jasonrhodes requested review from a team as code owners August 23, 2019 21:29
@jasonrhodes jasonrhodes added the release_note:skip Skip the PR/issue when compiling release notes label Aug 23, 2019
@jasonrhodes jasonrhodes removed the request for review from a team August 23, 2019 21:31
@jasonrhodes jasonrhodes self-assigned this Aug 23, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

jasonrhodes
jasonrhodes commented Aug 24, 2019
View reviewed changes
...egacy/plugins/apm/public/components/app/ServiceDetails/ServiceIntegrations/WatcherFlyout.tsx
@@ -83,7 +83,8 @@ export class WatcherFlyout extends Component<
WatcherFlyoutProps,
WatcherFlyoutState
> {
static contextType = CoreContext;
static contextType = KibanaCoreContext;
context!: React.ContextType<typeof KibanaCoreContext>;
Copy link
Member Author

@jasonrhodes jasonrhodes Aug 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT this is the way to let TS know what type this.context should be in the rest of the class, so that each reference to this.context doesn't need to self-type.

It's weird and just another reason to eventually move all our shit away from classes :)

jasonrhodes
jasonrhodes commented Aug 24, 2019
View reviewed changes
...k/legacy/plugins/apm/public/components/app/ServiceOverview/__test__/ServiceOverview.test.tsx
@@ -11,7 +11,7 @@ import { toastNotifications } from 'ui/notify';
import * as callApmApi from '../../../../services/rest/callApmApi';
import { ServiceOverview } from '..';
import * as urlParamsHooks from '../../../../hooks/useUrlParams';
import * as coreHooks from '../../../../hooks/useCore';
import * as kibanaCore from '../../../../../../observability/public/context/kibana_core';
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, we only export things from observability/public by way of Kibana convention, but if you try to import from there and then use jest.spyOn(), it blows up because the re-exported stuff in the public/index file doesn't mesh with jest.spyOn.

For more info, see: https://stackoverflow.com/a/53307822

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

sorenlouv
sorenlouv reviewed Aug 26, 2019
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv 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.

This LGTM 👍
I'll defer approval to @ogupte since he is the one working on platform migration on the APM side.

ogupte
ogupte approved these changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

This is great, glad that we can share this hook. Should we remove the APM-specific useCore and CoreContext code in this PR or a follow-up?

@jasonrhodes
Copy link
Member Author

This is great, glad that we can share this hook. Should we remove the APM-specific useCore and CoreContext code in this PR or a follow-up?

I'll do it now, thanks!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jasonrhodes jasonrhodes merged commit eeff5ef into elastic:master Aug 27, 2019
@jasonrhodes jasonrhodes mentioned this pull request Aug 27, 2019
Merged
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Aug 29, 2019
@jasonrhodes
[7.x] [APM] Sets up APM with new shared Kibana core context (elastic#…
6961a16 
…43920)
jasonrhodes added a commit that referenced this pull request Sep 3, 2019
@jasonrhodes
[7.x] [APM] Sets up APM with new shared Kibana core context (#43920) (#…
7e482e1 
…44139)

* [7.x] [APM] Sets up APM with new shared Kibana core context (#43920)

* Backporting shared observability plugin to 7.x so it can be used going forward

* Removing rogue strange folder added to SIEM app during cherry pick
@jasonrhodes jasonrhodes mentioned this pull request Sep 3, 2019
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv left review comments

@ogupte ogupte ogupte approved these changes

Assignees

@jasonrhodes jasonrhodes

Labels
release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasonrhodes @elasticmachine @sorenlouv @ogupte