feat: drilldown selection feature added to cartesian chart #1149

jyothishjose6190 · 2021-09-27T05:42:32Z

brush added to cartesian chart
on a feature**: brush added to cartesian chart

codecov · 2021-09-27T05:46:19Z

Codecov Report

Merging #1149 (5abad38) into main (2ea76f4) will decrease coverage by 0.06%.
The diff coverage is 70.09%.

❗ Current head 5abad38 differs from pull request most recent head 4be3739. Consider uploading reports for the commit 4be3739 to get more accurate results

@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
- Coverage   84.53%   84.47%   -0.07%     
==========================================
  Files         843      847       +4     
  Lines       17932    18032     +100     
  Branches     2357     2369      +12     
==========================================
+ Hits        15159    15232      +73     
- Misses       2662     2687      +25     
- Partials      111      113       +2

Impacted Files	Coverage Δ
...components/src/popover/popover-position-builder.ts	`76.27% <0.00%> (-2.68%)`	⬇️
projects/components/src/popover/popover.ts	`100.00% <ø> (ø)`
...s/apis/api-detail/metrics/api-metrics.dashboard.ts	`100.00% <ø> (ø)`
...apis/api-detail/overview/api-overview.dashboard.ts	`100.00% <ø> (ø)`
...ackend-detail/metrics/backend-metrics.dashboard.ts	`100.00% <ø> (ø)`
...kend-detail/overview/backend-overview.component.ts	`40.00% <ø> (ø)`
...ervice-detail/metrics/service-metrics.dashboard.ts	`100.00% <ø> (ø)`
...vice-detail/overview/service-overview.dashboard.ts	`100.00% <ø> (ø)`
...y/src/pages/explorer/explorer-dashboard-builder.ts	`89.04% <ø> (ø)`
...red/components/cartesian/d3/data/cartesian-data.ts	`75.00% <0.00%> (-10.72%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ea76f4...4be3739. Read the comment docs.

jyothishjose6190 · 2021-10-06T17:42:17Z

@anandtiwary @itssharmasandeep please review this

projects/observability/src/shared/components/cartesian/cartesian-chart.component.ts

projects/observability/src/shared/components/cartesian/d3/chart/cartesian-chart.ts

.../src/shared/dashboard/widgets/charts/cartesian-widget/cartesian-widget-renderer.component.ts

.../widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation-handler.model.ts

anandtiwary · 2021-10-06T23:49:25Z

.../widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation-handler.model.ts

+  @ModelInject(NavigationService)
+  private readonly navigationService!: NavigationService;
+
+  public execute(dateRange: any): Observable<void> {


this can be typed

@anandtiwary I have made changes except for typed. . please review.

.../widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation-handler.model.ts

jyothishjose6190 · 2021-10-19T03:53:21Z

@aaron-steinfeld @anandtiwary please review

aaron-steinfeld

A bit confused, but should probably have one of the UI folks look at it (I rarely work in the UI code any more)

projects/common/src/time/time-range.service.ts

aaron-steinfeld · 2021-10-22T21:31:19Z

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

+  private readonly navigationService!: NavigationService;
+
+  // tslint:disable-next-line
+  public execute(selectionData: any): Observable<void> {


please remove disable and fix type

aaron-steinfeld · 2021-10-22T21:32:04Z

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

+
+  // tslint:disable-next-line
+  public execute(selectionData: any): Observable<void> {
+    const startPoint = selectionData[0];


Do we have a guarantee we'll get two data points here?

changed to timerange

aaron-steinfeld · 2021-10-22T21:34:14Z

projects/observability/src/shared/components/cartesian/d3/chart/cartesian-chart.ts

 import { TimeRange } from '@hypertrace/common';
-import { ContainerElement, mouse, select } from 'd3-selection';
+import { brush, BrushBehavior, D3BrushEvent } from 'd3-brush';
+// tslint:disable-next-line: no-restricted-globals weird tslint error. Rename event so we can type it and not mistake it for other events


do we need to lead with the underscore here?

aaron-steinfeld · 2021-10-22T21:39:26Z

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

+    const startPoint = selectionData[0];
+    const endPoint = selectionData[1];
+
+    // When there is only one point in selected area, start timestamp  and end timestamp is same. This causes timerange exception


Hmm - this kind of makes me wonder what the expected result of the selection actually is. In many products, a selection on the time series is independent of the data entirely (e.g. if I select from 9 am -> 11 am, that becomes my time range even if the data points are at 10am and 1030am).

Would it be better to represent a selection object something like:

interface Selection { selectedRange: TimeRange; // I actually can't recall, are all of our cartesians time based or do they support any range type? selectedData: Map<Series, Data[]>; }

aaron-steinfeld · 2021-10-22T21:40:29Z

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

+    const endPoint = selectionData[1];
+
+    // When there is only one point in selected area, start timestamp  and end timestamp is same. This causes timerange exception
+    const startTime = new Date(startPoint.dataPoint.timestamp).getTime() - 6000;


This is assuming the time part I Was asking about above, but capturing this in the selection object would prevent the need for this work around (and give a more accurate selection not limited by the current data).

aaron-steinfeld · 2021-10-22T21:42:35Z

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

+      path: ['/explorer'],
+      queryParams: params,
+      queryParamsHandling: 'merge',
+      replaceCurrentHistory: true


it goes to explorer with no filter? That seems very unintuitive. I guess it's a completely different set of functionality, but I'd expect a range selection to apply a time range. Also unclear on the merge params. The params from the source page may not apply to the explorer.

Why would this replace history? Should have default history support.

uses custom time range to filter data between the selected time ranges

projects/observability/src/shared/components/cartesian/cartesian-chart.component.ts

projects/observability/src/shared/components/cartesian/cartesian-chart.module.ts

projects/observability/src/shared/components/cartesian/d3/chart/cartesian-chart.ts

...et/interactions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.ts

.../src/shared/dashboard/widgets/charts/cartesian-widget/cartesian-widget-renderer.component.ts

anandtiwary · 2021-11-10T17:15:04Z

...et/interactions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.ts

+    </div>
+  `
+})
+export class CartesianExplorerContextMenuComponent {


Just use this component inside interaction handler. No need to make this a generic component as of now.

anandtiwary · 2021-11-15T19:56:05Z

@jyothishjose6190 when can we incorporate the remaining review comments?

jyothishjose6190 · 2021-11-16T02:26:31Z

@jyothishjose6190 when can we incorporate the remaining review comments?

I will incorporate them today.

anandtiwary · 2021-11-23T06:55:28Z

projects/observability/src/shared/components/cartesian/cartesian-chart.component.ts

-      );
+      )
+      .withEventListener(ChartEvent.Select, selectedData => {
+        this.selectionChange.emit(selectedData as CartesianSelectedData<TData>);


Is this cast required? Can we type it so that selectedData come as CartesianSelectedData<TData> ?

may be we can do if(data instanceOf CartesianSelectedData<TData>) ?

@jyothishjose6190 looks like you missed this

itssharmasandeep · 2021-11-25T10:22:50Z

@itssharmasandeep - Can you have a look at the earliest?

@jyothishjose6190 is making some changes, once he's done, I will take a look.

jaywalker21 · 2021-12-27T09:49:55Z

@jyothishjose6190 Where do we stand on this?
Can we merge this if all is good?

anandtiwary · 2021-12-28T08:06:34Z

LGTM

anandtiwary

apart from few minor outstanding issues, this is good to go. @aaron-steinfeld @itssharmasandeep your thoughts?

aaron-steinfeld · 2021-12-28T22:48:27Z

apart from few minor outstanding issues, this is good to go. @aaron-steinfeld @itssharmasandeep your thoughts?

My thoughts haven't changed since the beginning - I don't think this is the right UX for the use case. A brush selection on the cartesian should change the time range only, like it does in most products. If we want to support drilling into explorer, that's independent of a brush selection. As I read this PR, a brush selection on a cartesian brings up a context menu that has one option. That option, "Explore" changes time range and navigates to explorer, but it doesn't maintain the context of the actual metrics on the chart in any way (and can't because many of the charts being displayed are not current exposed via explorer, at least not directly). Trying to change the time range itself without navigating to drill into the data in context is a significantly more common use case. If we had built that without a context menu (e.g. how grafana works), then it would still take the same number of clicks to get to the trace screen with the time range - one drag to set the time range, and one click to switch to explorer.

I do recognize that this has been open for months at this point, but I've probably brought this up a half dozen times in various contexts.

I think the hard part is done here - the brush selection - but I'd like to see us streamline the UX still, or at a minimum offer the option to just set the time range.

anandtiwary · 2021-12-29T18:36:40Z

@aaron-steinfeld I agree with the concern here, but this ux had been discussed in the past with various stakeholders and we decided to go ahead with this approach for hypertrace. We will surely iterate over this

aaron-steinfeld · 2021-12-29T20:48:30Z

@aaron-steinfeld I agree with the concern here, but this ux had been discussed in the past with various stakeholders and we decided to go ahead with this approach for hypertrace. We will surely iterate over this

Discussed this with @anandtiwary and came to a good compromise. For context, my concern is for Hypertrace in that I don't think in its current form we solve either ticket (if there's any others that I missed, please clarify) :

[Feature request] Ability to drill down to a time range from aggregations hypertrace#220 - where the ask is to apply the global time range in context like grafana, as I was describing above. In the current PR, we lose context because the only way to set the global time range on brush selection also navigtes.
Correlate Service and API dashboard metrics with related Traces hypertrace#208 - where the ask is to correlate traces corresponding to a particular chart. Here, the problem is we're not bringing the context with us when we navigate to explorer, we're just setting the global time range and going to explorer with a blank slate.

As a compromise that both maintains the existing work and helps better align to the use cases with minimal extra effort, my proposal is to add a "Set Time Range" option to the context menu as the first option before "Explore", whose affect is to just set the time range without navigating. With that hypertrace/hypertrace#220 is solved I believe. As to hypertrace/hypertrace#208 , we can keep iterating on that in later PRs to bring more context with us when clicking Explore to accomplish the described use case.

jyothishjose6190 · 2022-01-03T05:28:45Z

@aaron-steinfeld I agree with the concern here, but this ux had been discussed in the past with various stakeholders and we decided to go ahead with this approach for hypertrace. We will surely iterate over this

Discussed this with @anandtiwary and came to a good compromise. For context, my concern is for Hypertrace in that I don't think in its current form we solve either ticket (if there's any others that I missed, please clarify) :

[Feature request] Ability to drill down to a time range from aggregations hypertrace#220 - where the ask is to apply the global time range in context like grafana, as I was describing above. In the current PR, we lose context because the only way to set the global time range on brush selection also navigtes.

Correlate Service and API dashboard metrics with related Traces hypertrace#208 - where the ask is to correlate traces corresponding to a particular chart. Here, the problem is we're not bringing the context with us when we navigate to explorer, we're just setting the global time range and going to explorer with a blank slate.

As a compromise that both maintains the existing work and helps better align to the use cases with minimal extra effort, my proposal is to add a "Set Time Range" option to the context menu as the first option before "Explore", whose affect is to just set the time range without navigating. With that hypertrace/hypertrace#220 is solved I believe. As to hypertrace/hypertrace#208 , we can keep iterating on that in later PRs to bring more context with us when clicking Explore to accomplish the described use case.

@anandtiwary @aaron-steinfeld made changes as discussed above. please review

...et/interactions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.ts

...teractions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.test.ts

...shared/dashboard/widgets/charts/cartesian-widget/cartesian-widget-renderer.component.test.ts

...et/interactions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.ts

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

...gets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.test.ts

aaron-steinfeld

Unresolved several comments that had been resolved but not addressed as well.

projects/common/src/time/time-range.service.test.ts

jyothishjose6190 · 2022-01-17T06:00:42Z

@aaron-steinfeld updated PR with your comments. Please check

projects/common/src/time/time-range.service.ts

...teractions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.test.ts

projects/observability/src/shared/components/cartesian/d3/chart/cartesian-chart.ts

...shared/dashboard/widgets/charts/cartesian-widget/cartesian-widget-renderer.component.test.ts

.../src/shared/dashboard/widgets/charts/cartesian-widget/cartesian-widget-renderer.component.ts

...teractions/cartesian-explorer-context-menu/cartesian-explorer-context-menu.component.test.ts

...gets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.test.ts

...d/widgets/charts/cartesian-widget/interactions/cartesian-explorer-selection-handler.model.ts

aaron-steinfeld

lgtm, didn't have a chance to review the bursh and cartesian code though, so relying on @anandtiwary 's review of that - @anandtiwary did you want to take one more pass or good to go?

anandtiwary · 2022-01-26T18:52:56Z

LGTM

anandtiwary · 2022-01-26T18:53:29Z

@jyothishjose6190 Could you please merge the latest main in this PR branch?

anandtiwary · 2022-01-26T19:23:18Z

@aaron-steinfeld something went wrong here.

This commit has all the changes.
4be3739

aaron-steinfeld · 2022-01-26T19:54:17Z

Looks like @jyothishjose6190 didn't merge main but instead force pushed main into his remote branch, overwriting all of the commits. Force pushing is very dangerous for this reason - I strongly discourage anyone from doing it unless you have a really strong understanding of git and are working solo such that rewriting history will not affect anyone else. Hopefully he still has the original commit on his local.

jyothishjose6190 · 2022-01-26T19:57:54Z

@anandtiwary @aaron-steinfeld Sorry about this. I will keep this in mind

aaron-steinfeld · 2022-01-26T20:01:26Z

It's your branch (looks like it still exists in the razorpay repo, so it's recoverable), you're welcome to do what you want with it - it's just riskier for the author to use force pushing in terms of data loss because it goes around version control and overwrites history each time.

jyothishjose6190 · 2022-01-26T20:20:30Z

new PR created with all the changes #1394

jyothishjose6190 changed the title ~~feat: brush dded to cartesian chart~~ feat: drilldown selection feature added to cartesian chart Oct 6, 2021