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

[SIEM] Add chart interactions - update date picker after brush selection on charts #42440

Merged
merged 4 commits into from
Aug 3, 2019

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Aug 1, 2019

Summary

Update date picker after brushing on a chart:
According to the documentation, we are able to pass a function to onBrushEnd, and therefore it will execute the function after brushing on a the chart. The function will recieve min and max selection of timestamp if it is a time chart (reference). So in this implementation, I just pass narrowDateRange all the way down to the chart and it is doing its job.

set-title

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@angorayc angorayc changed the title [SIEM] Chart interactions [SIEM] Add chart interactions Aug 1, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/secops

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

narrowDateRange={(min: number, max: number) => {
setTimeout(() => {
setAbsoluteRangeDatePicker({ id: 'global', from: min, to: max });
}, 500);
Copy link
Contributor Author

@angorayc angorayc Aug 1, 2019

Choose a reason for hiding this comment

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

Wait a bit here as chart updates the state for itself when BrushEnd. If we updated the date picker immediately after brushEnd, we would have unmountd the chart for fetching data before setState completed.
set-title-2

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish I had a better suggestion for this, but FWIW as a user, the delay doesn't feel perceivable.

Copy link
Member

Choose a reason for hiding this comment

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

I think this a-ok as is as the UI doesn't feel laggy or anything, but I wonder what other ways we can get around this?

Two that come to mind would be:

  • Use narrowDateRange to set a flag that can be used as the dependency for a useEffect that runs after the state update.
  • If there's a way to cancel/prevent said state update from propagating within narrowDateRange.

Either way, just thoughts -- doesn't appear to be any need to make any modifications :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't think of these options! Those are very interesting ideas, but I haven't had it work yet 😢, I'll follow up the update from charts and improve this accordingly.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@angorayc
Copy link
Contributor Author

angorayc commented Aug 1, 2019

jenkins, test this please

@angorayc angorayc changed the title [SIEM] Add chart interactions [SIEM] Add chart interactions - update date picker after brush selection on charts Aug 1, 2019
@angorayc angorayc marked this pull request as ready for review August 1, 2019 18:56
yTickFormatter: numberFormatter,
},
settings: {
onBrushEnd: getOr(() => {}, 'onBrushEnd', config),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@angorayc
Copy link
Contributor Author

angorayc commented Aug 2, 2019

jenkins, test this please

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

This is awesome! 😎 I ran this branch locally and interacted with it for ~ 30 minutes, and it behaved as-expected every single time. It feels good to be able to, for example, see a spike in authentication failures and then brush to see what's happening.
LGTM 📈


const chartHeight = 74;
export const chartDefaultRotation: Rotation = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be exported? I don't see any usages outside this file currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, let me remove that, Thanks @spong ! You've got eagle eyes!
eagle eyes

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally, and performed code review. Nice clean implementation + tests @angorayc! LGTM! 👍:tada:🚀

I've left a few comments, but no changes required.

In testing I found these two ancillary issues:

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@angorayc angorayc merged commit 392b8e1 into elastic:master Aug 3, 2019
angorayc added a commit to angorayc/kibana that referenced this pull request Aug 3, 2019
…ion on charts (elastic#42440)

* add click and brush events

* replace setAbsoluteRangeDatePicker

* fix type error

* remove a not necessary export
angorayc added a commit that referenced this pull request Aug 3, 2019
…ion on charts (#42440) (#42559)

* add click and brush events

* replace setAbsoluteRangeDatePicker

* fix type error

* remove a not necessary export
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 5, 2019
…-or-edit-existing-rollup-job

* 'master' of github.com:elastic/kibana: (67 commits)
  [TSVB] Shim new platform (elastic#39169)
  [Metric Vis] Shim new platform (elastic#42240)
  [Tag Cloud] Shim new platform (elastic#42348)
  Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS.
  Add disk space percentage to node listing (elastic#42145)
  [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440)
  Document HTTP service (elastic#42331)
  [Reporting] Sanitize 409 error log message (elastic#42495)
  [docs][skip ci] Maps read only access (elastic#35561)
  [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407)
  [DOCS] Updates images and content in Dashboard docs (elastic#42500)
  Allow sorting on multiple columns in Discover (elastic#41918)
  [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287)
  [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534)
  Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298)
  Increase max-old-space-size for builds (elastic#42218)
  [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836)
  [Logs UI][a11y] Announce name of column on remove column button (elastic#41695)
  Inspector 👉 New Platform (elastic#42164)
  Make alerting properly space aware (elastic#42081)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 5, 2019
…s_autocomplete

* 'master' of github.com:elastic/kibana: (189 commits)
  [TSVB] Shim new platform (elastic#39169)
  [Metric Vis] Shim new platform (elastic#42240)
  [Tag Cloud] Shim new platform (elastic#42348)
  Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS.
  Add disk space percentage to node listing (elastic#42145)
  [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440)
  Document HTTP service (elastic#42331)
  [Reporting] Sanitize 409 error log message (elastic#42495)
  [docs][skip ci] Maps read only access (elastic#35561)
  [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407)
  [DOCS] Updates images and content in Dashboard docs (elastic#42500)
  Allow sorting on multiple columns in Discover (elastic#41918)
  [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287)
  [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534)
  Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298)
  Increase max-old-space-size for builds (elastic#42218)
  [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836)
  [Logs UI][a11y] Announce name of column on remove column button (elastic#41695)
  Inspector 👉 New Platform (elastic#42164)
  Make alerting properly space aware (elastic#42081)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants