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

[APM] Service inventory redesign #76744

Merged
merged 30 commits into from
Sep 14, 2020
Merged

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Sep 4, 2020

Summary

Closes #75252.

Before:
image

After:
image

@dgieselaar dgieselaar added release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0 labels Sep 4, 2020
export function getBucketSize(
start: number,
end: number,
numBuckets: number = 100
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed for the spark plots, where we should use less buckets because of how small the plot is.

bucketSize: minBucketSize,
intervalString: interval,
bucketSize: 0,
intervalString: 'auto',
Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find any instances where we were using anything else than 'auto', so I hardcoded the value into the function.


const services = await getServices({
setup,
mlAnomaliesEnvironment: environment,
Copy link
Member Author

Choose a reason for hiding this comment

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

Using mlAnomaliesEnvironment to make it more explicit that this should only be used for fetching anomalies, as it's already part of uiFilters in other cases.

@dgieselaar dgieselaar marked this pull request as ready for review September 4, 2020 09:39
@dgieselaar dgieselaar requested a review from a team as a code owner September 4, 2020 09:39
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

First of all - this looks so good 😍

Made a small suggestion to the tooltip on the service name in the list.

Besides this, I found that the sparkline charts for TPM and Error rate both dip at the end. I think I recall we shifting these in our regular charts?

Screenshot 2020-09-04 at 13 06 53

Additionally, I wonder how it's possible to chart the error rate for opbeans-rum when its error rate chart is empty. Looks like it's a replicate of the TPM chart.

Screenshot 2020-09-04 at 13 12 07

Screenshot 2020-09-04 at 13 12 14

{
field: 'serviceName',
name: i18n.translate('xpack.apm.servicesTable.nameColumnLabel', {
defaultMessage: 'Name',
}),
width: '40%',
sortable: true,
render: (serviceName: string) => (
render: (_, { serviceName, agentName }) => (
<EuiToolTip content={formatString(serviceName)} id="service-name-tooltip">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<EuiToolTip content={formatString(serviceName)} id="service-name-tooltip">
<EuiToolTip delay="long" content={formatString(serviceName)} id="service-name-tooltip">

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to delay the tooltip on the service name (since we're showing it always for truncation). It's always been a pet peeve of mine and the way we're truncating these values in our tables.

return useFetcher(
(callApmApi) =>
callApmApi({
pathname: `/api/apm/settings/anomaly-detection`,
Copy link
Member

Choose a reason for hiding this comment

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

In a previous PR review I asked Oliver to change this to

Suggested change
pathname: `/api/apm/settings/anomaly-detection`,
pathname: `/api/apm/settings/anomaly-detection/jobs`,

to make it obvious that it returned the jobs and to align it with POST /api/apm/settings/anomaly-detection/jobs.
However, he ran in to some typescript issues (it was not possible to have the same route twice even for different http methods).
Would you mind looking into it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about fixing this when we can use template string types. We could index route definitions as POST /api/apm/settings/anomaly-detection/jobs and GET /api/apm/settings/anomaly-detection/jobs. I expect this to be easier to implement than resolving this specific issue with the options we have today. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's fine with me 👍

Comment on lines 325 to 336
return Object.keys(anomalies.serviceAnomalies).reduce<
Array<{ serviceName: string; severity?: Severity }>
>((prev, serviceName) => {
const stats = anomalies.serviceAnomalies[serviceName];

const severity = getSeverity(stats.anomalyScore);

return prev.concat({
serviceName,
severity,
});
}, []);
Copy link
Member

@sorenlouv sorenlouv Sep 4, 2020

Choose a reason for hiding this comment

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

I think I commented on something similar recently: can you rewrite with [].map?

Edit: map/filter should always be preferred over reduce To explain why I find map/filter preferable over reduce: With map, without reading your implementation, I know that an array with the same length will be returned. With filter I know the length might change but the items will be identical.
reduce comes with barely any guarantees, since mostly anything can be returned. This forces the reader to spent more time understanding the implementation because it's not possible to assume much.

Copy link
Contributor

Choose a reason for hiding this comment

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

map/filter should always be preferred over reduce

Not sure I agree with this. Maybe when transforming and array to another array.

reduce comes with barely any guarantees, since mostly anything can be returned

What does that mean? In this case I know an Array<{ serviceName: string; severity?: Severity }> is being returned.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I agree with this. Maybe when transforming and array to another array.

But that's exactly it. When transforming an array to another array and keeping the same length, that's what map is for.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, [].reduce has its place, like when you want to sum the items in an array (number[] => number). But transformations like T[] => T[] and T[] => P[] almost always become simpler with [].filter and/or [].map.

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, this is an oversight on my part. It should have been a .map. I think I just default to reduce out of habit when using Object.keys().

const mlJobIds = await getMLJobIds(
setup.ml.anomalyDetectors,
uiFilters.environment
);
Copy link
Member

Choose a reason for hiding this comment

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

Better 👍

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

Agree with most of the comments from Soren and Casper, just here to say this is gorgeous and I love it.

addWarning,
<EuiThemeProvider>
<MockApmPluginContextWrapper
value={
Copy link
Contributor

Choose a reason for hiding this comment

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

This crap is way easier if you use immer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think something like defaults would help here as well. Will check.

@dgieselaar
Copy link
Member Author

@formgeist I was still using errors per minute. Now that Cauê's work for event.outcome is merged, I've replaced it with the transaction error rate. I've also updated the screenshots. I'm not sure if we handle the "dip" differently in other charts, @sqren are you aware of any custom handling there?

@dgieselaar
Copy link
Member Author

@elasticmachine merge upstream

@dgieselaar
Copy link
Member Author

@sqren I've optimistically removed io-ts and added value tests, under the assumption that #77229 lands in some form in the near future.

defaultMessage: 'Critical',
}
);
break;
Copy link
Member

Choose a reason for hiding this comment

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

nit: break no longer necessary

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

I added a single nit but you can ignore that since you've got a green build and fixing that doesn't justify holding this up any longer.

Comment on lines +60 to +70
expectSnapshot(severityScores).toMatchInline(`
Array [
undefined,
undefined,
undefined,
undefined,
undefined,
"warning",
undefined,
]
`);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to filter out undefined severity scores up front:

Suggested change
expectSnapshot(severityScores).toMatchInline(`
Array [
undefined,
undefined,
undefined,
undefined,
undefined,
"warning",
undefined,
]
`);
const severityScores = response.body.items.map((item: any) => item.severity).filter(Boolean);
expect(severityScores.length).to.be.greaterThan(0);
expectSnapshot(severityScores).toMatchInline(`
Array [
"warning",
]
`);

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that would make sense, but it might no longer be necessary when we fix #77083. I've added it as a note there, to be able to merge this in a bit.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 1245 +6 1239
observability 90 +1 89
total +7

async chunks size

id value diff baseline
apm 4.9MB +36.3KB 4.9MB
observability 296.6KB -291.0B 296.9KB
total +36.0KB

miscellaneous assets size

id value diff baseline
apm 49.0KB -39.1KB 88.1KB

page load bundle size

id value diff baseline
apm 41.7KB -51.0B 41.7KB
observability 51.7KB +1.9KB 49.9KB
total +1.8KB

distributable file count

id value diff baseline
default 45514 -4 45518

History

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

@dgieselaar dgieselaar merged commit 520d348 into elastic:master Sep 14, 2020
@dgieselaar dgieselaar deleted the service-inventory branch September 14, 2020 14:10
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (26 commits)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  [ML] Functional tests - increase wait time for DFA start (elastic#77307)
  [UiActions][Drilldowns] Fix actions sorting in context menu (elastic#77162)
  [Drilldowns] Wire up new links to new docs (elastic#77154)
  Fix APM issue template
  [Ingest Pipelines] Drop into an empty tree (elastic#76885)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
dgieselaar added a commit that referenced this pull request Sep 15, 2020
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@cauemarcondes cauemarcondes self-assigned this Oct 14, 2020
@cauemarcondes
Copy link
Contributor

Tested on all browsers.

Opened some bugs:
#80472
#80473
#80475
#80482

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
7 participants