Skip to content

Commit

Permalink
[ML] Single Metric Viewer: Fix time bounds with custom strings. (elas…
Browse files Browse the repository at this point in the history
…tic#55045)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
  • Loading branch information
walterra authored and jkelastic committed Jan 17, 2020
1 parent 9cbb683 commit 2ba74d2
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ interface Duration {

function getRecentlyUsedRangesFactory(timeHistory: TimeHistory) {
return function(): Duration[] {
return timeHistory.get().map(({ from, to }: TimeRange) => {
return {
start: from,
end: to,
};
});
return (
timeHistory.get()?.map(({ from, to }: TimeRange) => {
return {
start: from,
end: to,
};
}) ?? []
);
};
}

Expand All @@ -54,9 +56,18 @@ export const TopNav: FC = () => {

useEffect(() => {
const subscriptions = new Subscription();
subscriptions.add(timefilter.getRefreshIntervalUpdate$().subscribe(timefilterUpdateListener));
subscriptions.add(timefilter.getTimeUpdate$().subscribe(timefilterUpdateListener));
subscriptions.add(timefilter.getEnabledUpdated$().subscribe(timefilterUpdateListener));
const refreshIntervalUpdate$ = timefilter.getRefreshIntervalUpdate$();
if (refreshIntervalUpdate$ !== undefined) {
subscriptions.add(refreshIntervalUpdate$.subscribe(timefilterUpdateListener));
}
const timeUpdate$ = timefilter.getTimeUpdate$();
if (timeUpdate$ !== undefined) {
subscriptions.add(timeUpdate$.subscribe(timefilterUpdateListener));
}
const enabledUpdated$ = timefilter.getEnabledUpdated$();
if (enabledUpdated$ !== undefined) {
subscriptions.add(enabledUpdated$.subscribe(timefilterUpdateListener));
}

return function cleanup() {
subscriptions.unsubscribe();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ export * from './new_job';
export * from './datavisualizer';
export * from './settings';
export * from './data_frame_analytics';
export * from './timeseriesexplorer';
export { timeSeriesExplorerRoute } from './timeseriesexplorer';
export * from './explorer';
export * from './access_denied';
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { MemoryRouter } from 'react-router-dom';
import { render } from '@testing-library/react';

import { I18nProvider } from '@kbn/i18n/react';

import { TimeSeriesExplorerUrlStateManager } from './timeseriesexplorer';

jest.mock('ui/new_platform');

describe('TimeSeriesExplorerUrlStateManager', () => {
test('Initial render shows "No single metric jobs found"', () => {
const props = {
config: { get: () => 'Browser' },
jobsWithTimeRange: [],
};

const { container } = render(
<I18nProvider>
<MemoryRouter>
<TimeSeriesExplorerUrlStateManager {...props} />
</MemoryRouter>
</I18nProvider>
);

expect(container.textContent).toContain('No single metric jobs found');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ interface TimeSeriesExplorerUrlStateManager {
jobsWithTimeRange: MlJobWithTimeRange[];
}

const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateManager> = ({
export const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateManager> = ({
config,
jobsWithTimeRange,
}) => {
Expand Down Expand Up @@ -102,23 +102,27 @@ const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateManager> =
timefilter.enableAutoRefreshSelector();
}, []);

// We cannot simply infer bounds from the globalState's `time` attribute
// with `moment` since it can contain custom strings such as `now-15m`.
// So when globalState's `time` changes, we update the timefilter and use
// `timefilter.getBounds()` to update `bounds` in this component's state.
const [bounds, setBounds] = useState<TimeRangeBounds | undefined>(undefined);
useEffect(() => {
if (globalState?.time !== undefined) {
timefilter.setTime({
from: globalState.time.from,
to: globalState.time.to,
});

const timefilterBounds = timefilter.getBounds();
// Only if both min/max bounds are valid moment times set the bounds.
// An invalid string restored from globalState might return `undefined`.
if (timefilterBounds?.min !== undefined && timefilterBounds?.max !== undefined) {
setBounds(timefilter.getBounds());
}
}
}, [globalState?.time?.from, globalState?.time?.to]);

let bounds: TimeRangeBounds | undefined;
if (globalState?.time !== undefined) {
bounds = {
min: moment(globalState.time.from),
max: moment(globalState.time.to),
};
}

const selectedJobIds = globalState?.ml?.jobIds;
// Sort selectedJobIds so we can be sure comparison works when stringifying.
if (Array.isArray(selectedJobIds)) {
Expand All @@ -140,14 +144,17 @@ const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateManager> =
}, [JSON.stringify(selectedJobIds)]);

// Next we get globalState and appState information to pass it on as props later.
// If a job change is going on, we fall back to defaults (as if appState was already cleard),
// If a job change is going on, we fall back to defaults (as if appState was already cleared),
// otherwise the page could break.
const selectedDetectorIndex = isJobChange
? 0
: +appState?.mlTimeSeriesExplorer?.detectorIndex || 0;
const selectedEntities = isJobChange ? undefined : appState?.mlTimeSeriesExplorer?.entities;
const selectedForecastId = isJobChange ? undefined : appState?.mlTimeSeriesExplorer?.forecastId;
const zoom = isJobChange ? undefined : appState?.mlTimeSeriesExplorer?.zoom;
const zoom: {
from: string;
to: string;
} = isJobChange ? undefined : appState?.mlTimeSeriesExplorer?.zoom;

const selectedJob = selectedJobIds && mlJobService.getJob(selectedJobIds[0]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,25 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Timefilter } from 'ui/timefilter';
import { FC } from 'react';

import { Timefilter } from 'ui/timefilter';

import { getDateFormatTz, TimeRangeBounds } from '../explorer/explorer_utils';

declare const TimeSeriesExplorer: FC<{
appStateHandler: (action: string, payload: any) => void;
autoZoomDuration?: number;
bounds?: TimeRangeBounds;
dateFormatTz: string;
jobsWithTimeRange: any[];
lastRefresh: number;
selectedJobIds: string[];
selectedDetectorIndex: number;
selectedEntities: any[];
selectedForecastId: string;
setGlobalState: (arg: any) => void;
tableInterval: string;
tableSeverity: number;
timefilter: Timefilter;
zoom?: { from: string; to: string };
}>;
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,10 @@ export class TimeSeriesExplorer extends React.Component {
selectedDetectorIndex: PropTypes.number,
selectedEntities: PropTypes.object,
selectedForecastId: PropTypes.string,
setGlobalState: PropTypes.func.isRequired,
tableInterval: PropTypes.string,
tableSeverity: PropTypes.number,
zoom: PropTypes.object,
};

state = getTimeseriesexplorerDefaultState();
Expand Down Expand Up @@ -481,7 +483,7 @@ export class TimeSeriesExplorer extends React.Component {
zoom,
} = this.props;

if (selectedJobIds === undefined) {
if (selectedJobIds === undefined || bounds === undefined) {
return;
}

Expand Down

0 comments on commit 2ba74d2

Please sign in to comment.