[APM] Use theme variables for components

[balthazar]

Summary

The current APM stacktrace display does not get the current theme variables but rather imports the light theme directly, making it quite unreadable on dark mode:

Assuming the ThemeProvider is setup correctly for APM and following other components examples using the theme prop, this is what the same stacktrace would look like:

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[cla-checker-service]

💚 CLA has been signed

[balthazar]

Not sure about the refresh time on your CLA bot, but it's signed

[elasticmachine]

Pinging @elastic/apm-ui (Team:apm)

[sorenlouv]

Thanks a lot @balthazar

Assuming the ThemeProvider is setup correctly for APM and following other components examples using the theme prop, this is what the same stacktrace would look like

We have an issue to setup the ThemeProvider. I'll try to get around to it today.

[sorenlouv]

retest

[sorenlouv]

Note to self: Should be merged after #68242

[balthazar]

@sqren Maybe I can try to solve more issues from #30048 (not just the stacktrace) through this PR so ease the work required after #68242 is good

[sorenlouv]

@balthazar that would be greatly appreciated!

[balthazar]

@sqren I got lost in translation and refactored every APM component (minus getServiceColors from waterfall_helpers.ts because of all the changes it would entail when the euiColorVisX variables do not seem to change between the two themes).

Just like other plugins seemed to import the withTheme and EuiTheme from observability/public, I opted for this solution to be consistent

I think some tests target the light theme colors so as long as these tests use the light theme setting from the ThemeProvider it should also be ok.

There's a lot of things I don't think I can test like the ServiceMap as our company is using the Open-Source plan and not a paid one. Speaking of it, I decided to update the cytoscapeOptions to use getters by passing the theme retrieved from the Map components, but let me know if you would like to do it another way!

That's a ton of changes so 🐻 with me! Also as a side note I would have loved to be able to import modules using their absolute paths rather than relative and have to count the level of nesting (especially on vim haha), but I know changing something like that for such a codebase is going to be tricky.

[sorenlouv]

This is simply amazing. Great work @balthazar!

I have one nit, that is not required for merging this PR but would be the neat: since we are moving away from higher-order-components (like withTheme) to hooks it would be great if that's possible to use in those cases. styled-components has the useTheme hook that we can use like:

import { useTheme } from 'styled-components';

function MyComponent() {
  const theme = useTheme();
  console.log('Current theme: ', theme);
  // ...
}

WDYT?

[balthazar]

Thanks! 😄

Yeah I also prefer hooks over the HoC, but since that's what I saw was being used most of the time I chose to use that since there would be less risk of it breaking stuff. I think in some cases there is still some classes though, so hooks would not work for those.

So I should be able to simply replace the withTheme imports from observability to useTheme of styled-components? I think I would still have to refactor the code to access theme.eui instead of theme if that's what you are going to pass in the ThemeProvider right?

[sorenlouv]

I think I would still have to refactor the code to access theme.eui instead of theme if that's what you are going to pass in the ThemeProvider right?

Oh yeah, you mean like?

  const theme = useTheme();
  console.log('Current theme: ', theme.eui);

[balthazar]

Yeah, alright going to start the refactor

[balthazar]

@sqren I think that should be it! 😃

While I was at it, I decided to convert VerticalLines and HistogramInner from classes to function components, and local state to hooks (#yolo)

I also did a subsequent commit to change the hardcoded white background color to euiColorEmptyShade since I think it would work better on the Map, but like I said unsure of it since I can't really see it 😁

Let me know!

[sorenlouv]

This looks great! Me or someone from the team will give this a test drive next week and merge.
Thank you for your contribution!

[sorenlouv]

retest

[sorenlouv]

@balthazar Can you try running the jest test? You might need to update the snapshots.

cd x-pack/plugins/apm && npx jest --updateSnapshot

[balthazar]

@sqren Updated the snapshots and fixed a syntax error I introduced, lots of the tests are obviously failing since I do not have the Provider setup

[sorenlouv]

I think you should pass in theme so we can use EuiTheme type consistently

Suggested change

	...getAnimationOptions(theme.eui),
	...getAnimationOptions(theme),

[balthazar]

@sqren You're right, I for some reason thought EuiTheme was the theme directly. Regarding the new hook I didn't do it because your eslint config was complaining about some public import stuff, didn't want to look too much into that.

[sorenlouv]

Regarding the new hook I didn't do it because your eslint config was complaining about some public import stuff, didn't want to look too much into that.

If you can post the error here I can try looking into it.

[balthazar]

Well @cauemarcondes asked me to change it to that so it should probably live in another PR as an improvement, because this is working effectively.

[sorenlouv]

The reason for having the useTheme hook is to improve the types. When doing the following the type for theme is any:

  const theme = useContext(ThemeContext);

[ogupte]

@elasticmachine merge upstream

[ogupte]

Added a commit which updates the service map style to be a little more friendly in dark mode:

Also fixes a bug where the service node border colors were not being set, and removed some code related to anomaly detection since it was updated recently to a new component.

[sorenlouv]

retest

[formgeist]

Added a commit which updates the service map style to be a little more friendly in dark mode

Thanks @ogupte - I've created an issue for optimizing the styles a bit for dark mode, not to block this PR.

[sorenlouv]

I just tried to take this for a spin and seeing this error on the first page of APM:

The error happens in

kibana/x-pack/plugins/apm/public/components/shared/EuiTabLink.tsx

Line 32 in 461d44d

!isSelected ? `color: ${theme.euiTextColor} !important;` : ''}

At first I thought this was simply fixed with theme.eui.euiTextColor - but it is theme which is undefined.

[sorenlouv]

retest

[sorenlouv]

All eslint and tsc issues in apm should now be fixed. 🤞 that this doesn't affect too many other plugins

[sorenlouv]

retest

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: beb918d
• Pipeline Steps (look for red circles / failed steps)
• Interpreting CI Failures

Failed CI Steps

• Execute kibana-intake

Build metrics

page load asset size

beta

id	value	diff	baseline
/bundles/plugin/apm/apm.plugin.js	215.6KB	+79.0B	215.5KB

History

• 💔 Build #54468 failed 69e21ab
• 💔 Build #54262 failed e8efe2d
• 💔 Build #53432 failed d988ec3
• 💔 Build #53393 failed 81bfa6a
• 💔 Build #52112 failed d973aa2d50ba7029ef24a96432278f2bbb5ea8b6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[formgeist]

Overall this looks great - I've updated our dark mode support issue to check all the fixes, and update the list of remaining issues. #30048