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

Fix issue with heatmap showing black tiles #20753

Merged
merged 4 commits into from
Aug 30, 2018
Merged

Fix issue with heatmap showing black tiles #20753

merged 4 commits into from
Aug 30, 2018

Conversation

guanghaofan
Copy link
Contributor

@guanghaofan guanghaofan commented Jul 13, 2018 •
edited by timroes
Loading

Fixes #20544, fixes #12420

the enhancement and the fix:

  1. Only re-quantized the heat map metric data point while the full data scale range is grate than the maximum allowed legend color number, current is 10.
  2. The heat map cell with value 'null' should always be hidden, this will eliminate the default balck color for those cells.
  3. the color bucket id should always grate than or equal 0 but not only exclude -1 for hidden case.

@elasticmachine
Copy link
Contributor

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?

@karmi
Copy link

karmi commented Jul 13, 2018

Hi @guanghaofan, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@guanghaofan
Copy link
Contributor Author

@karmi just added the commit email, thank you for your reminder!

@jbudz jbudz added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Jul 16, 2018
@jbudz
Copy link
Member

jbudz commented Jul 16, 2018

/cc @elastic/kibana-visualizations

@timroes timroes requested review from timroes and markov00 July 16, 2018 16:28
@timroes timroes changed the title Issue/20544 Fix issue with heatmap showing black tiles Jul 16, 2018
@timroes
Copy link
Contributor

timroes commented Jul 16, 2018

Jenkins, test this

@timroes timroes added Feature:Heatmap Heatmap visualization release_note:fix labels Jul 16, 2018
@timroes
Copy link
Contributor

timroes commented Jul 18, 2018

Jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jul 18, 2018

@guanghaofan Actually the PR seems to fail linting rules. That should automatically be checked on commiting actually 🤔 You can run linting (in case your editor doesn't do it automatically) by using node scripts/eslint.

@guanghaofan
Copy link
Contributor Author

@timroes I will update it soon! thanks!

@elastic elastic deleted a comment from elasticmachine Aug 6, 2018
@elastic elastic deleted a comment from elasticmachine Aug 6, 2018
@markov00
Copy link
Member

markov00 commented Aug 6, 2018

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@markov00
Copy link
Member

markov00 commented Aug 6, 2018

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@markov00
Copy link
Member

@guanghaofan can you please rebase on master to see if the CI test pass?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@guanghaofan
Copy link
Contributor Author

yarn test result

1375 passing (1m)
5 pending
5 failing

  1. dev/build/lib/fs copyAll() copies files and directories from source to dest, creating dest if necessary, respecting mode:
    Error: expected '664' to equal '644'
    at Assertion.assert (node_modules/expect.js/index.js:96:13)
    at Assertion.be.Assertion.equal (node_modules/expect.js/index.js:216:10)
    at Assertion.(anonymous function) [as be] (node_modules/expect.js/index.js:69:24)
    at Context.it (src/dev/build/lib/tests/fs.js:229:73)
    at

  2. createOrUpgradeSavedConfig() "before all" hook:
    Error: ES exited with code 1
    at exports.createCliError (packages/kbn-es/src/errors.js:21:17)
    at ChildProcess._process.once.code (packages/kbn-es/src/cluster.js:195:18)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)

  3. createOrUpgradeSavedConfig() "after all" hook:
    Error: ES exited with code 1
    at exports.createCliError (packages/kbn-es/src/errors.js:21:17)
    at ChildProcess._process.once.code (packages/kbn-es/src/cluster.js:195:18)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)

  4. uiSettings/routes "before all" hook: startServers:
    Error: ES exited with code 1
    at exports.createCliError (packages/kbn-es/src/errors.js:21:17)
    at ChildProcess._process.once.code (packages/kbn-es/src/cluster.js:195:18)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)

  5. uiSettings/routes "after all" hook: stopServers:
    Error: ES exited with code 1
    at exports.createCliError (packages/kbn-es/src/errors.js:21:17)
    at ChildProcess._process.once.code (packages/kbn-es/src/cluster.js:195:18)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)

Warning: non-zero exit code 5 Use --force to continue.

but can pass the
yarn build

│ succ ✓ 0 sec

info [ kibana ] Transpiling SCSS to CSS
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana/src/core_plugins/status_page/public/index.scss
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana/node_modules/x-pack/plugins/monitoring/public/index.scss
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana/node_modules/x-pack/plugins/dashboard_mode/public/index.scss
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana/src/core_plugins/kibana/public/index.scss
│ succ ✓ 14 sec

info [kibana-oss] Transpiling SCSS to CSS
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana-oss/src/core_plugins/status_page/public/index.scss
│ info Compiled SCSS: /home/ghfan/Documents/dev/kibana/build/kibana-oss/src/core_plugins/kibana/public/index.scss
│ succ ✓ 3 sec

info [ kibana ] Cleaning tests, examples, docs, etc. from node_modules
│ succ ✓ 3 min 53 sec

info [kibana-oss] Cleaning tests, examples, docs, etc. from node_modules
│ succ ✓ 3 min 1 sec

info [ kibana ] Running optimizer
│ info Running bin/kibana to trigger the optimizer
│ succ ✓ 2 min 55 sec

info [kibana-oss] Running optimizer
│ info Running bin/kibana to trigger the optimizer
│ succ ✓ 1 min 43 sec

info [ kibana ] Creating platform-specific archive source directories
...
...

@markov00
Copy link
Member

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@guanghaofan
Copy link
Contributor Author

so why failing on those tests? and I can not see the details of the online build?
It' seems the failures are not related to this PR changes based on my offline testing.

@markov00 markov00 self-assigned this Aug 30, 2018
@markov00
Copy link
Member

@guanghaofan sorry for the delay and long wait on this PR get merged.
So the failures are not related to your changes. Let me check and I will be back in few hours.

@markov00
Copy link
Member

retest

markov00
markov00 approved these changes Aug 30, 2018
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM. Waiting for CI to pass after merging master.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Aug 30, 2018

Jenkins, test this - seems like a flaky test (cc @stacey-gammon ):

"dashboard app using legacy data dashboard state Overriding colors on an area chart is preserved"
             │      Error: Expected to find "saveDashboardSuccess" toast after saving dashboard

On the screenshot the toast is visible.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00 markov00 merged commit f6a3f90 into elastic:master Aug 30, 2018
markov00 pushed a commit to markov00/kibana that referenced this pull request Aug 30, 2018
@guanghaofan @markov00
Fix issue with heatmap showing black tiles (elastic#20753)
d8deea1 
* Heatmap metric data points should only be quantized while the data full range is grate than the maxmum allowed color count

* The data cell with metric value 'null' should always be hidden in the heatmap

* Add one variable for max color count and follow the coding style
@markov00 markov00 mentioned this pull request Aug 30, 2018
Merged
@timroes
Copy link
Contributor

timroes commented Aug 30, 2018

@guanghaofan Thanks a lot for fixing this issue. We merged this PR now on master and 6.x, so it will be released with 6.5.0.

markov00 added a commit that referenced this pull request Aug 30, 2018
@markov00
Fix issue with heatmap showing black tiles (#20753) (#22535)
2a190be 
* Heatmap metric data points should only be quantized while the data full range is grate than the maxmum allowed color count

* The data cell with metric value 'null' should always be hidden in the heatmap

* Add one variable for max color count and follow the coding style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees

@markov00 markov00

Labels
Feature:Heatmap Heatmap visualization Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v6.5.0 v7.0.0
Projects
None yet
Milestone
No milestone
6 participants
@guanghaofan @elasticmachine @karmi @jbudz @timroes @markov00