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

[TSVB] Math Aggregation (two point oh) #16965

Merged
merged 13 commits into from
Apr 11, 2018
Merged

[TSVB] Math Aggregation (two point oh) #16965

merged 13 commits into from
Apr 11, 2018

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Mar 4, 2018 •
edited
Loading

This PR reverts the "Remove MathJS Feature" and replaces mathjs with tinymath

Closes #16958

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@shaharmor
Copy link
Contributor

Haleluya

@simianhacker simianhacker added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Feature:TSVB TSVB (Time Series Visual Builder) labels Mar 7, 2018
@simianhacker simianhacker requested a review from epixa March 7, 2018 17:40
mattapperson
mattapperson approved these changes Mar 7, 2018
View reviewed changes
Copy link
Contributor

@mattapperson mattapperson left a comment

Choose a reason for hiding this comment

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

LGTM

shaharmor
shaharmor reviewed Mar 7, 2018
View reviewed changes
package.json Outdated
@@ -199,6 +199,7 @@
"tabbable": "1.1.0",
"tar": "2.2.0",
"tinygradient": "0.3.0",
"tinymath": "^0.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be made static

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa
Copy link
Contributor

epixa commented Mar 10, 2018

Nice! There are conflicts on this PR though.

@epixa epixa mentioned this pull request Mar 10, 2018
Merged
@epixa
Copy link
Contributor

epixa commented Mar 10, 2018

Will this recognize saved objects that were created with the old mathjs feature?

@simianhacker simianhacker mentioned this pull request Mar 22, 2018
Closed
@alexfrancoeur
Copy link

Nice!! @simianhacker @epixa will this make 6.3?

@simianhacker
Copy link
Member Author

@epixa Good catch, I'm going to change the key to tinymath. I also noticed that the UnsupportedAgg component got lost. I'm going to add that back in as well.

@epixa
Copy link
Contributor

epixa commented Mar 22, 2018

@simianhacker To be clear, I was asking specifically because it would be convenient for folks that had used the old feature to be able to upgrade and have this new feature power their previous visualizations that used math aggs. The math featureset might not be 100% compatible, so it wouldn't be a perfect upgrade experience, but in theory it would allow them to go in and fix their existing math aggs in the event that they stopped working. Does that make sense? Do you agree?

@simianhacker
Copy link
Member Author

@epixa Gotcha... I misinterpreted your comment, to answer you're question now that I understand where you're coming from. Yes, this uses the same aggregation type so it will allow them to re-write their equations (if it's more complex then just basic math). Most things should just work (fingers crossed).

@epixa
Copy link
Contributor

epixa commented Mar 22, 2018

Awesome. There was at least one user that created 100s of visualizations using math aggs in 6.1.0, so it's at least good they'll have a path forward that doesn't involve exporting, find+replacing on the raw json, and reimporting.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar requested review from ppisljar and removed request for timroes April 6, 2018 13:58
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Adding functional test for math aggregation
65fefa5 
- Fixing bad tests
- Resetting page for every test suite (why donesn't beforeEach work?)
- Adding test for Math agg
@ppisljar
Copy link
Member

ppisljar commented Apr 6, 2018

you can try running the tests locally and limit them to just your test suite so you don't have to wait an hour to see if it fails.

@simianhacker
Copy link
Member Author

@ppisljar There were two things going on with the tests:

  1. They didn't run consistently between my local environment and the remote (CI) environment. It would be totally green on my end but as soon as I uploaded it to the CI server it would fail. This was mostly due to some timing issues.

  2. The whole test suite was poorly designed; instead of resetting the page for every suite, the test would run on the last state of the page. As soon as, I added my test the remaining tests would fail. This wasn't obvious the first 4 times I ran it (due to the amount of output in our testing), once I realized the issue then I struggled to get beforeEach to work (and it still doesn't).

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

Jenkins test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

QUICK! Review this sucker before it turns RED again.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ppisljar
ppisljar approved these changes Apr 11, 2018
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@simianhacker simianhacker merged commit 7fed7b6 into elastic:master Apr 11, 2018
simianhacker added a commit that referenced this pull request Apr 11, 2018
@simianhacker
[TSVB] Math Aggregation (two point oh) (#16965)
84ad08e 
* Revert "Remove MathJS Feature (#15652)"

This reverts commit 43bf1db.

* replacing math with tinymath

* pining version

* updating yarn.lock

* Fixing Prettier mis formatting

* fixing tests

* Adding unsupported agg back

* Adding functional test for math aggregation

- Fixing bad tests
- Resetting page for every test suite (why donesn't beforeEach work?)
- Adding test for Math agg

* Trying to fix values (due to inconsistencies in env)
@simianhacker
Copy link
Member Author

simianhacker commented Apr 11, 2018 •
edited
Loading

Back ported to 6.x with 84ad08e

@simianhacker simianhacker deleted the tinymath branch April 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck left review comments

@shaharmor shaharmor shaharmor left review comments

@mattapperson mattapperson mattapperson approved these changes

@ppisljar ppisljar ppisljar approved these changes

@epixa epixa Awaiting requested review from epixa

Assignees
No one assigned
Labels
Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.3.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@simianhacker @elasticmachine @shaharmor @epixa @alexfrancoeur @thomasneirynck @ppisljar @mattapperson