fix(heatmap): snap time bucket to calendar/fixed intervals by markov00 · Pull Request #1462 · elastic/elastic-charts

markov00 · 2021-11-04T19:17:07Z

Summary

The PR introduced the ability to specify both calendars and fixed intervals as described in Elasticserach date_histogram aggregation

This fix was required to cover the Lens usage when calendar intervals are used in conjunction with the date_histogram aggs.

BREAKING CHANGE

The xScaleType is replaced by the prop xScale, which better describes and collects the information related to the passed interval if any.

These are the exposed types as part of the API changes

interface HeatmapSpec extends Spec {
  ...
  xScale: RasterTimeScale | OrdinalScale | LinearScale;
  ...
}

export interface TimeScale {
  type: typeof ScaleType.Time;
}

export interface RasterTimeScale extends TimeScale {
  interval: ESCalendarInterval | ESFixedInterval;
}

export interface LinearScale {
  type: typeof ScaleType.Linear;
}

export interface OrdinalScale {
  type: typeof ScaleType.Ordinal;
}
interface ESFixedInterval {
  type: 'fixed';
  unit: ESFixedIntervalUnit;
  value: number;
}

interface ESCalendarInterval {
  type: 'calendar';
  unit: ESCalendarIntervalUnit;
  value: number;
}

type ESFixedIntervalUnit = 'ms' | 's' | 'm' | 'h' | 'd';

type ESCalendarIntervalUnit =
  | 'minute'
  | 'm'
  | 'hour'
  | 'h'
  | 'day'
  | 'd'
  | 'week'
  | 'w'
  | 'month'
  | 'M'
  | 'quarter'
  | 'q'
  | 'year'
  | 'y';

Details

The histogram chart needs to discretize the domain if a time scale is passed in the X axis. To correctly discretize based on the domain min, max values, we need to apply a specific rounding formula depending on timezone and type of interval (calendar/fixed). The discretization is required to cover the case of sparse datasets.

Issues

fix #1165

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
All related issues have been linked (i.e. closes #123, fixes #123)
New public API exports have been added to packages/charts/src/index.ts
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated

Add dataset and updated stories

nickofthyme

changes LGTM

A side note for the future. I wonder if some of the ES types are available in @elastic/elasticsearch that we can leverage.

monfera · 2021-11-08T11:55:20Z

packages/charts/api/charts.api.md

+// @public (undocumented)
+export type ESCalendarIntervalUnit = 'minute' | 'm' | 'hour' | 'h' | 'day' | 'd' | 'week' | 'w' | 'month' | 'M' | 'quarter' | 'q' | 'year' | 'y';


Not necessarily in this PR, we could eventually depend on types in elasticsearch.js as an authoritative source

Just to clarify, as we have an elasticsearch.ts, I meant, at some point directly devDepending on https://github.com/elastic/elasticsearch-js for these types. This way, if elasticsearch adds a new unit, and we bump the dependency, we'll get automatic static errors saying that we don't cover the new unit option(s)

monfera · 2021-11-08T12:01:23Z

packages/charts/api/charts.api.md

 export type HeatmapElementEvent = [Cell, SeriesIdentifier];

+// @alpha (undocumented)
+export interface HeatmapNonTimeScale {


Minor: A positively stated scale name would work better, if possible. The word "scalar" implies unitlessness but it doesn't look nice next to "Scale", so, maybe, UnitlessScale?

The word "Heatmap" in the scale name represents tight coupling, because it casts the Scale as a property of (subsidiary to) the chart type Heatmap, while they're at least equal, orthogonal things or even, Heatmap is subsidiary to the Scale. Of course I see how you want to rule out logarithmic etc. scales.

Maybe UnitlessHeatmapScale is an OK alternative

or I can completely remove that named type and use something like:

xScale: | HeatmapTimeScale | { type: typeof ScaleType.Ordinal | typeof ScaleType.Linear; };

wdyt?

with your consideration should also HeatmapTimeScale be renamed to TimeHeatmapScale?

Part 1: I'm fine with that too, though in terms of syntax, it combines two types of unioning, where it could be just one, eg. { type: ordinal | linear | time } or xScale: Heatmap | Ordinal | Linear

Part 2: Neither name is something I really prefer, as both of them tightly bind a scale type to a chart type maybe needlessly, though the first option rolls off the tongue more easily to me. But the best would be, if possible, to not have a chart type name in the name of a scale type. So, something like TimeScale, or however we call it in xy already

packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts

monfera · 2021-11-08T12:11:47Z

packages/charts/src/chart_types/heatmap/state/selectors/get_heatmap_table.ts

+function isFiniteNumber(value: number | undefined): value is number {
+  return Number.isFinite(value);
+}


Nice! I hope TS will fix this oversight eventually. As it's currently an abstraction that's leaking from hundreds of self-inflicted wounds, Number.isFinite is one of those things that should narrow types (here, to number if the result is true), and this utility is a nice workaround in the meantime

I know, I was following your comments on the TS repo and I was thinking that, for now, can solve our cases

Maybe it could be a value: unknown, and it can be put into common or utils, as there are many places where I temporarily used a typeof d === 'number' && Number.isFinite(d) check where isFiniteNumber would be clearer. All places where we do === 'number' is an easy to grep candidate for a future PR. Even the name of this function is perfect 👌

monfera

LGTM! We discussed two improvements:

long term: possible multilayer axis (after it had been extracted from xy)
shorter term follow-up PR: avoiding tick overlap

nickofthyme

LGTM, I don't see any issues after playing with the new story for any of the datasets. Old stories work as before.

packages/charts/src/utils/chrono/elasticsearch.ts

# [39.0.0](v38.1.5...v39.0.0) (2021-11-09) ### Bug Fixes * **deps:** update dependency @elastic/eui to v41 ([#1468](#1468)) ([0c38291](0c38291)) * **heatmap:** snap time bucket to calendar/fixed intervals ([#1462](#1462)) ([b76c12c](b76c12c)) * **xy:** handle zero-length time domains and switch to 24hr time ([#1464](#1464)) ([379c2d6](379c2d6)) ### BREAKING CHANGES * **heatmap:** The `xScaleType` is replaced by the prop `xScale`, which better describes a rasterized time scale with an Elasticsearch compliant interval.

nickofthyme · 2021-11-09T08:30:03Z

🎉 This PR is included in version 39.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 added 5 commits November 4, 2021 19:50

Fix the heatmap time discretization

e99af4c

Update API specs

4f6e7c5

Fix basic heatmap story

5c16f84

Fix time story

7c89a57

Align both ESInterval to interfaces

eb00c5d

markov00 added bug Something isn't working :axis Axis related issue :data Data/series/scales related issue breaking change :heatmap Heatmap/Swimlane chart related issue labels Nov 4, 2021

Fix test used types

bc0fffd

markov00 mentioned this pull request Nov 5, 2021

[heatmap] Fix x time scale with calendar intervals elastic/kibana#117619

Merged

1 task

markov00 added the wip work in progress label Nov 5, 2021

Fix the fixed interval time range

100bf5b

Add dataset and updated stories

nickofthyme approved these changes Nov 5, 2021

View reviewed changes

markov00 added 2 commits November 5, 2021 16:35

Filter duplicated x values around DST

296d46e

Fix missing timezone in computing the fixedTimeRange

500f58f

markov00 requested a review from nickofthyme November 5, 2021 17:18

Fix the brush to cover gte:start to lt:end

7a0d7db

markov00 removed the wip work in progress label Nov 5, 2021

Fix linting errors

4057868

markov00 requested a review from monfera November 7, 2021 15:18

markov00 added 2 commits November 8, 2021 10:05

Use UTC formatting on heatmap time story

f623e80

Simplify story data

59882e3

monfera reviewed Nov 8, 2021

View reviewed changes

packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts Outdated Show resolved Hide resolved

monfera reviewed Nov 8, 2021

View reviewed changes

packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts Show resolved Hide resolved

monfera reviewed Nov 8, 2021

View reviewed changes

monfera self-requested a review November 8, 2021 13:04

monfera approved these changes Nov 8, 2021

View reviewed changes

nickofthyme approved these changes Nov 8, 2021

View reviewed changes

packages/charts/src/utils/chrono/elasticsearch.ts Show resolved Hide resolved

markov00 added 3 commits November 8, 2021 17:41

Better types for heatmap scales

0eb451a

Move back to the original position the timeZone param

ad7c7d0

Update chart API report

985e3fc

markov00 merged commit b76c12c into elastic:master Nov 9, 2021

nickofthyme added the released Issue released publicly label Nov 9, 2021

markov00 mentioned this pull request Apr 13, 2022

[Heatmap] Time scale type with a single row of data breaks the chart #1184

Closed

		// @public (undocumented)
		export type ESCalendarIntervalUnit = 'minute' \| 'm' \| 'hour' \| 'h' \| 'day' \| 'd' \| 'week' \| 'w' \| 'month' \| 'M' \| 'quarter' \| 'q' \| 'year' \| 'y';

Conversation

markov00 commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

BREAKING CHANGE

Details

Issues

Checklist

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

monfera Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

markov00 Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

monfera Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markov00 Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickofthyme commented Nov 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markov00 commented Nov 4, 2021 •

edited

Loading

monfera Nov 8, 2021 •

edited

Loading