XY Chart: Add area and scatter chart styles #399

thefinaljob · 2021-06-22T03:00:28Z

Added area and scatter chart styles through a dropdown menu

Contributes towards fixing #45

MatthewKhouzam

Initial comment

https://www.freecodecamp.org/news/writing-good-commit-messages-a-practical-guide/

MatthewKhouzam · 2021-06-22T12:44:55Z

Initial hot take:

Setting the default to scatter is not a great idea. offering scatter is.
Using a dropdown may not be as interesting as using toggle buttons
Scatters show latencies better
The UI freezes in Linux/Firefox with this patch. Not sure if it's related.
You need to sign off your patches if you want them to pass DCO
Stacked area makes sense for very few views, I would omit it from the 1st patch.
Most importantly, this is really cool!

Initial action items:

Fix your commit message
Make line the default
Use toggles (icons can be https://fontawesome.com/v5.15/icons/chart-line?style=regular)
Remove the stacked area as it implies a total which is not the case often
Have a github issue associated to the commit message

I only looked at the functionality, not the code, so this can be considered a "sanity check"

MatthewKhouzam · 2021-06-22T12:46:05Z

Also funny bug, the more I resize the view and play with it, the smaller the dropdown gets.

ebugden · 2021-06-22T17:30:16Z

I recently learned that each data series in an xy chart specifies the chart type/style (line, scatter) that should be used to visualize it. I think this PR should be adjusted to automatically display data based on the chart type specified in the series label rather than offering a drop down like I initially suggested to @thefinaljob. Allowing all chart types implies that any option makes sense which isn't the case (ex. you wouldn't use a scatter plot to visualize continuous time-based data). @MatthewKhouzam @tahini thoughts?

tahini · 2021-06-22T18:27:38Z

I recently learned that each data series in an xy chart specifies the chart type/style (line, scatter) that should be used to visualize it. I think this PR should be adjusted to automatically display data based on the chart type specified in the series label rather than offering a drop down like I initially suggested to @thefinaljob. Allowing all chart types implies that any option makes sense which isn't the case (ex. you wouldn't use a scatter plot to visualize continuous time-based data). @MatthewKhouzam @tahini thoughts?

Yes! That was indeed discussed offline with Nikolai. Note that the data type from the server comes with the series (for instance, I think the CPU usage has the total as a line and individual threads as area), not the whole chart, while iiuc, with chartjs, the series type will have to be for the whole chart. Would you default to the first one you get?

tahini · 2021-06-22T18:35:26Z

packages/react-components/src/components/xy-output-component.tsx

-            this.buildXYData(xyDataResponse.model.series);
+            switch (this.desiredChartStyle) {
+                case 'scatter':
+                    this.buildScatterData(xyDataResponse.model.series);


As discussed offline, if different types of chart require the data to be formatted differently, they should be encapsulated together in a single class so that there can be no mismatch. You could add separate widget classes for each series type (in react, it is suggested to multiply small stateless classes instead of big ones with ton of stuff in it). So you could have ScatterXy, LineXy, AreaXy, which would take the series as props and would format the data appropriately and render the proper line type.

Here, you could definitely have a race condition between changing the chart type and zooming in with the old chart type and get the view to temporarily render wrong, until it is rendered right ;-)

But I'll let the maintainers of the extension decide if my comment is purely informative or if you should act on it

thefinaljob · 2021-08-18T00:59:11Z

Reopeing after a snafu

thefinaljob · 2021-08-18T01:01:52Z

This is a mess, this is what happens when you try to fix solder.... several times.

thefinaljob · 2021-08-18T01:12:00Z

OK looks like the submission is ready. To address @MatthewKhouzam's comments.

After discussing with ebugden, we are keeping the dropdown as is, using a toggle for each individual chart style would take up too much real estate. Also, having a toggle between a chart and table style would not be optimal given that a lot of the time there isn't a table view available
We cannot have the chart style be chosen automatically yet as the API is sending a "null" value. The endpoint exists but it is not usable as of yet. I will issue a bug report regarding this.
Line has been made default
The UI freeze typically happens when too many data points are chosen, this could be fixed by allowing for a max number of data points to visualized
After discussing with ebugden, the stacked area chart will stay. I forgot the reasons though @ebugden can chime in here :)
Changes have been made to address issues with my previous commits

Thank you for your feedback Matthew!! This was my first serious PR and it was great to get a lot of good pointers in the right direction.

Known bugs

Border is not set to 0 when changing from line to stacked area, after talking to @tahini it seems that only dividing each of these chart styles into separate components would fix the issue as they would be destroyed and re-initialized upon a change in the chart style.

…pse-cdt-cloud#45 Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>

MatthewKhouzam · 2021-08-18T12:29:09Z

Hey, before getting into the review proper, please remember to update the commit message to follow the guidelines.

The reasoning being if and when something goes wrong, it's jarring to have a Hodge-podge of commit messages to go through, standardized ones will make things clearer in our time of duress. :)

MatthewKhouzam · 2021-08-18T13:42:44Z

Stacked area charts freeze on my computer. Could you profile it with the apt trace CPU usage and clicking on all?

ebugden · 2021-08-18T13:43:15Z

Hey, before getting into the review proper, please remember to update the commit message to follow the guidelines.

The reasoning being if and when something goes wrong, it's jarring to have a Hodge-podge of commit messages to go through, standardized ones will make things clearer in our time of duress. :)

@thefinaljob the commit guidelines were recently added to the main readme in the How to contribute code section.

MatthewKhouzam

Please update the commit message and fix the performance issue.

mirsky-work · 2021-08-18T20:38:13Z

I just tested the functionality on Gitpod - very cool!
A nitpicking - right now the [X, Y] values' label is shown when selecting a scatter chart element. Can we show it on mouse hover?

ebugden · 2021-09-08T14:46:30Z

Exceptionally, I'm going to make edits to the title and description of this PR to correct some style issues.

bhufmann · 2021-10-20T21:03:46Z

@thefinaljob @ebugden any news on this one. It would interesting to continue with it on it. It could help solving the issues with some of the line charts (e.g. 'Latency vs Time' that are scatter charts)?

By the way, this PR needs to rebased to the latest master branch where multiple updates to the XYOutputComponent have been done (i.e. zoom/navigation improvement, BigInt for timestamps). I pushed a rebased version to my GitHub fork (https://github.com/bhufmann/theia-trace-extension/tree/xy-chart-styles-rebased) where you can see the changes needed. Please note that I didn't address any review comments provided here.

Please note, that showing the 'Latency vs Time' views as Scatter chart work well and would improve the UX. Showing 'Latency vs Time' view as line charts with the current implementation doesn't not show the data correctly. They should be shown as Scatter chart where the points are connected with lines. This is an option to the scatter chart. This fix is not part of the scope of this PR.

ebugden · 2021-10-29T19:56:44Z

any news on this one.

@arfio could take a look at this if they have free time.

When selecting a "latency vs. time" view, the data is plotted in a scatter chart. Each point of the data set can be displayed individually (scatter) or connected by a line. This pull request replaces eclipse-cdt-cloud#399. Contributes towards fixing eclipse-cdt-cloud#45. Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>

Data sets with scattered points, like the "latency vs. time" view, are now plotted in a scatter chart. The type of data set is defined by the string "scatter" in the output id. Each point of the data set can be displayed individually (scatter) or connected by a line. This pull request replaces eclipse-cdt-cloud#399. Contributes towards fixing eclipse-cdt-cloud#45. Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>

Data sets with scattered points, like the "latency vs. time" view, are now plotted in a scatter chart. The type of data set is defined by the string "scatter" in the output id. The scatter chart is to remain not avialble to the user until the tooltip is implemented. This pull request replaces eclipse-cdt-cloud#399. Contributes towards fixing eclipse-cdt-cloud#45. Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>

Data sets with scattered points, like the "latency vs. time" view, are now plotted in a scatter chart. The type of data set is defined by the string "scatter" in the output id. The scatter chart is to remain not available to the user until the tooltip is implemented. This pull request replaces eclipse-cdt-cloud#399. Contributes towards fixing eclipse-cdt-cloud#45. Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>

Data sets with scattered points, like the "latency vs. time" view, are now plotted in a scatter chart. The type of data set is defined by the string "scatter" in the output id. The scatter chart is to remain not available to the user until the tooltip is implemented. This pull request replaces #399. Contributes towards fixing #45. Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>

bhufmann · 2021-12-21T18:29:56Z

Closing this one after #595 was merged. Thanks for the effort it helped with the current implementation.

MatthewKhouzam reviewed Jun 22, 2021

View reviewed changes

thefinaljob requested review from tahini and ebugden June 22, 2021 16:06

tahini reviewed Jun 22, 2021

View reviewed changes

ebugden mentioned this pull request Jun 30, 2021

Hide "analyses" that do not provide value to the user #296

Closed

thefinaljob closed this Aug 18, 2021

thefinaljob force-pushed the chartstyles branch from 5422129 to 820176f Compare August 18, 2021 00:25

thefinaljob reopened this Aug 18, 2021

thefinaljob force-pushed the chartstyles branch from 34fb923 to c0d2efa Compare August 18, 2021 01:18

added scatter and stacked area chart styles as outlined in issue ecli…

c8f79d8

…pse-cdt-cloud#45 Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>

thefinaljob force-pushed the chartstyles branch from c0d2efa to c8f79d8 Compare August 18, 2021 01:19

MatthewKhouzam requested changes Aug 18, 2021

View reviewed changes

ebugden changed the title ~~added area and scatter chart styles~~ XY Chart: Add area and scatter chart styles Sep 8, 2021

bhufmann mentioned this pull request Oct 21, 2021

Remove scatter data providers (latency scatter charts) #508

Merged

Rodrigoplp-work mentioned this pull request Dec 7, 2021

Scatter chart for latency vs. time views #595

Merged

bhufmann closed this Dec 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XY Chart: Add area and scatter chart styles #399

XY Chart: Add area and scatter chart styles #399

thefinaljob commented Jun 22, 2021 •

edited by ebugden

Loading

MatthewKhouzam left a comment

MatthewKhouzam commented Jun 22, 2021

MatthewKhouzam commented Jun 22, 2021

ebugden commented Jun 22, 2021 •

edited

Loading

tahini commented Jun 22, 2021

tahini Jun 22, 2021

thefinaljob commented Aug 18, 2021

thefinaljob commented Aug 18, 2021

thefinaljob commented Aug 18, 2021 •

edited by ebugden

Loading

MatthewKhouzam commented Aug 18, 2021

MatthewKhouzam commented Aug 18, 2021

ebugden commented Aug 18, 2021

MatthewKhouzam left a comment

mirsky-work commented Aug 18, 2021

ebugden commented Sep 8, 2021 •

edited

Loading

bhufmann commented Oct 20, 2021 •

edited

Loading

ebugden commented Oct 29, 2021

bhufmann commented Dec 21, 2021

XY Chart: Add area and scatter chart styles #399

XY Chart: Add area and scatter chart styles #399

Conversation

thefinaljob commented Jun 22, 2021 • edited by ebugden Loading

MatthewKhouzam left a comment

Choose a reason for hiding this comment

MatthewKhouzam commented Jun 22, 2021

MatthewKhouzam commented Jun 22, 2021

ebugden commented Jun 22, 2021 • edited Loading

tahini commented Jun 22, 2021

tahini Jun 22, 2021

Choose a reason for hiding this comment

thefinaljob commented Aug 18, 2021

thefinaljob commented Aug 18, 2021

thefinaljob commented Aug 18, 2021 • edited by ebugden Loading

MatthewKhouzam commented Aug 18, 2021

MatthewKhouzam commented Aug 18, 2021

ebugden commented Aug 18, 2021

MatthewKhouzam left a comment

Choose a reason for hiding this comment

mirsky-work commented Aug 18, 2021

ebugden commented Sep 8, 2021 • edited Loading

bhufmann commented Oct 20, 2021 • edited Loading

ebugden commented Oct 29, 2021

bhufmann commented Dec 21, 2021

thefinaljob commented Jun 22, 2021 •

edited by ebugden

Loading

ebugden commented Jun 22, 2021 •

edited

Loading

thefinaljob commented Aug 18, 2021 •

edited by ebugden

Loading

ebugden commented Sep 8, 2021 •

edited

Loading

bhufmann commented Oct 20, 2021 •

edited

Loading