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

Events Feature Phase 0: Replace Rhythm Charts with Elastic Charts #45114

Closed
wants to merge 4 commits into from

Conversation

igoristic
Copy link
Contributor

@igoristic igoristic commented Sep 9, 2019

EDIT: (09/10/2019) This phase will be to replace current legacy charts with Elastic Charts for all rhythmic pages

A test page can be viewed at: Stack Monitoring > Elasticsearch Nodes > node > Advanced


Todo:

  • Create a TS environment
  • Create elastic-charts component https://github.com/elastic/elastic-charts that will work with our current rhythm pages
  • Create types for Series, Metrics, Data, timeRange, etc
  • Fix styling:
    • Spacing between legend values
    • Solid dots on the pivots
    • Correct line colors (test with Dark mode)
    • Content hight to grow dynamically
  • Add scrub functionality: zoom/zoom-out functionality https://elastic.github.io/elastic-charts/?path=/story/interactions--brush-selection-tool-on-linear
  • Sync crosshair / mouse-movement to adjacent charts
  • Swap the old charts with the new elastic-charts component on all the monitoring charts pages
  • Remove old/legacy chart component
  • Add descriptive comments with parameter and return type descriptions format abiding to js docs
  • Add unit tests

@igoristic igoristic added the Team:Monitoring Stack Monitoring team label Sep 9, 2019
@igoristic igoristic self-assigned this Sep 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@igoristic igoristic changed the title Events Feature phase 1 Events Feature Phase 0 Sep 9, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

ycombinator commented Sep 10, 2019

Hey @igoristic, thanks for putting up this draft PR with a clear checklist of work to be done. If I'm understanding the checklist correctly, it seems that part of the work here is to replace the Rhythm implementation of charts in Stack Monitoring with an Elastic Charts implementation? Then, on top of that work, you'd need to do more work to implement the actual Events feature?

If my understanding is right, how do you feel about breaking out the first part of the work (replacing the charts implementation) into its own PR? That way we can get the benefit of that change to users even sooner (i.e. without having to wait on the Events work), plus it would reduce the scope of the PR and make it easier to review as well.

@igoristic igoristic changed the title Events Feature Phase 0 Events Feature Phase 0: Replace Rhythm Charts with Elastic Charts Sep 10, 2019
@igoristic
Copy link
Contributor Author

@ycombinator

How do you feel about breaking out the first part of the work (replacing the charts implementation) into its own PR?

I love that idea, and was actually considering it at some point. I guess this can be the "Phase 0" thats already in the title.

@cachedout
Copy link
Contributor

cachedout commented Sep 11, 2019

@igoristic Take a look at the charts here with annotations. It looks pretty similar to what we're thinking about doing. WDYT? #33558

@igoristic
Copy link
Contributor Author

Take a look at the charts here with annotations. It looks pretty similar to what we're thinking about doing. WDYT?

@cachedout I recently removed the following checkboxes:

As you can see vertical annotation is actually a standard feature in Elastic Charts. I have a version of this current branch with synthetic annotation events which works pretty well. The idea is that single lines will be used for individual events, and area annotation will be used for clustering events (if there are too many to show for a particular area). But, this will all be fleshed out in the next phase

I stripped out all the vertical annotation for now since it's reserved for Phase 1

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor

Hey @igoristic

I'm wondering if we should pick this up for 7.8, after finishing the migration of the client to new platform.

How much effort do you think is left here?

@igoristic
Copy link
Contributor Author

...
How much effort do you think is left here?

@chrisronline Sorry just started noticing notifications now, seems I was unsubscribed for some reason.

I would love to pick this up after the "Additional Alerts" are in 7.10. So, this could be the 7.11 effort, if you don't need any help with ECS stuff (or anything else). This should be a quick switch though, since the EUI chart component is pretty much done

@legrego legrego closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Monitoring Stack Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants