-
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
[APM] Experimental Service Map front end #46497
Conversation
💔 Build Failed |
8f1bef6
to
8f08116
Compare
💔 Build Failed |
8f08116
to
63938c8
Compare
💔 Build Failed |
2a02b6c
to
b569b56
Compare
💔 Build Failed |
💔 Build Failed |
040f6ba
to
655094a
Compare
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
d2665d1
to
d01c161
Compare
💔 Build Failed |
4349fd4
to
b668ed7
Compare
💔 Build Failed |
💔 Build Failed |
@smith Looks great so far. Wondering if we can add a background on the labels so they don't mix with the arrow lines making them hard to read? In the design I'm using the |
And the updated background pattern that looks a little more subtle than the current https://codesandbox.io/s/service-maps-background-zuzlk |
Adding these options for the node style: 'text-background-color': theme.euiColorLightestShade,
'text-background-opacity': 1,
'text-background-padding': theme.paddingSizes.xs,
'text-background-shape': 'roundrectangle', additional options are here if you want to suggest more tweaks to the labels: http://js.cytoscape.org/#style/labels |
c20c4f8
to
a234686
Compare
Updated in a2346863502. |
Updated in a2346863502. |
💔 Build Failed |
Thanks for the updates @smith - it's really starting to look great 🎉 |
a234686
to
cf547b3
Compare
💔 Build Failed |
💔 Build Failed |
It's great to see Cytoscape being used here. The work is looking good. Big props from the graph group. |
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.
Renovate changes look good, not sure about adding PSF to the license whitelist but relying on @peterschretlen to authorize
💚 Build Succeeded
|
src/core/public/mocks.ts
Outdated
@@ -54,12 +55,13 @@ function createCoreSetupMock() { | |||
} | |||
|
|||
function createCoreStartMock() { | |||
const mock: MockedKeys<CoreStart> = { | |||
const mock: MockedKeys<LegacyCoreStart> = { |
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.
This mock incorrect. injectedMetadata
is not exposed to the new platform plugin, but legacy plugins only. We decided to provide it for a while until ConfigService is done #47319
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.
So what should I do if I need to mock injectedVars
?
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.
name: RouteName.SINGLE_SERVICE_MAP | ||
} | ||
); | ||
} |
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.
Any reason '/services/:serviceName/service-map'
is added unconditionally but these are not?
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.
That was actually a duplicate from resolving a merge conflict. Fixed in 4f4b094.
|
||
useEffect(() => { | ||
if (cy) { | ||
cy.on('zoom', event => { |
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.
Is this event listener removed again? (to avoid memory leaks)
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.
Thanks for catching this. I forgot to put it there. It's been added in 4f4b094.
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 looked at that commit and I don't see anything related (it's reverting a mock change).
What I'd expect is the useEffect
body to return a cleanup function that calls cy.off('zoom', handler)
or something like 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.
Sorry no it was 799b5e9.
Add service map tabs on the main APM screen and for individual services. This is not yet hooked up to work with back-end data, so it always shows the same hard-coded graph. This is experimental, so you must have x-pack.apm.serviceMapEnabled: true in your Kibana config for it to show up. Also add "PSF" to the list of allowed licenses since a new dependency added uses this license (it's on the [green list](https://github.com/elastic/open-source/blob/master/elastic-product-policy.md#green-list).) Fixes elastic#44890 Fixes elastic#44853
💔 Build Failed |
💚 Build Succeeded |
@restrry I reverted the change you commented on earlier. @sqren I updated the things you mentioned. Thanks! |
@spalger The PSF license is a pre-approved (green-list) license, so we are OK to add it to the whitelist. |
Add service map tabs on the main APM screen and for individual services. This is not yet hooked up to work with back-end data, so it always shows the same hard-coded graph. This is experimental, so you must have x-pack.apm.serviceMapEnabled: true in your Kibana config for it to show up. Also add "PSF" to the list of allowed licenses since a new dependency added uses this license (it's on the [green list](https://github.com/elastic/open-source/blob/master/elastic-product-policy.md#green-list).) Fixes elastic#44890 Fixes elastic#44853
Add service map tabs on the main APM screen and for individual services. This is not yet hooked up to work with back-end data, so it always shows the same hard-coded graph. This is experimental, so you must have x-pack.apm.serviceMapEnabled: true in your Kibana config for it to show up. Also add "PSF" to the list of allowed licenses since a new dependency added uses this license (it's on the [green list](https://github.com/elastic/open-source/blob/master/elastic-product-policy.md#green-list).) Fixes #44890 Fixes #44853
Add service map tabs on the main APM screen and for individual services.
This is not yet hooked up to work with back-end data, so it always shows the same hard-coded graph.
This is experimental, so you must have
x-pack.apm.serviceMapEnabled: true
in your Kibana config for it to show up.Also add "PSF" to the list of allowed licenses since a new dependency added uses this license (it's on the green list.)
Fixes #44890
Fixes #44853