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] Move chartsSelector transformation to backend #26117

Merged

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Nov 23, 2018

This builds on top of #25848 and is the final refactor for now. Trust me :)

Excluding tests and snapshots the actual diff is: +456 (additions) and -424 (deletions). If you wonder how I got those numbers check out this gist.

On a high level this moves a lot of the transformations previously done on the frontend in chartSelectors to the backend.
This is because we already do a lot of transformations on the backend, and some things were actually duplicated due to this. Now all of the data transformations happen on the backend, and the new chartSelectors is all about presentation data (legend title, color etc.)

This has been thoroughly manually tested by side-by-side comparing it to the current implementation on cloud.

Move apmTimeseries to backend

Fix tests
@sorenlouv sorenlouv force-pushed the move-ml-anomaly-series-transform-to-backend branch from 6ec7c2d to 632f40b Compare November 23, 2018 10:46
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

}));

return (
<AreaSeries
<VerticalRectSeries
Copy link
Member Author

@sorenlouv sorenlouv Nov 24, 2018

Choose a reason for hiding this comment

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

This line might be the biggest change in this PR: going from AreaSeries to RectSeries.
An AreaSeries take points like { x, y0, y } - so no horisontal width. To set the horisontal width I used to add a starting point, an ending point, and then padded with a null point to avoid connecting the area with the following area (if there are no null values between them two points will be connected with a line). Pretty hacky:

acc.push({ x: p.x, y: 1 });
if (nextPoint.x == null || nextPoint.x > endX) {
acc.push(
{
x: endX,
y: 1
},
{
x: getX(p.x, 2)
}
);
}
return acc;
}, []);

RectSeries take points like { x0, x, y0, y }. So the hacky stuff can go away.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

As best I can tell this LGTM, although I'm not sure if we can guarantee that all the transforms that were happening on the front-end are accurately recreated on the back-end without some kind of end to end test that I don't think we have. That said, I'm happy to have moved all the transformations to one place...

@sorenlouv
Copy link
Member Author

without some kind of end to end test that I don't think we have

Agree. We should really have e2e tests for this.

sorenlouv added a commit that referenced this pull request Nov 28, 2018
…[APM] Get rid of `pre` middleware (#26256) (#26328)

* [APM] Move chartsSelector transformation to backend (#26117)

* [APM] Move ML anomaly transformation to backend

Move apmTimeseries to backend

Fix tests

* Update default values

* Fix bug

* [APM] Get rid of `pre` middleware (#26256)

Rename apmIndexPattern to apmIndexPatternTitle and narrow down search query

Fix tests

Remove unused aggregation

Revert "Rename apmIndexPattern to apmIndexPatternTitle and narrow down search query"

This reverts commit 5aa86744a0b360ceb75a59ebc8a0a084b24fbe50.
@sorenlouv sorenlouv deleted the move-ml-anomaly-series-transform-to-backend branch January 21, 2019 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants