Skip to content

Commit

Permalink
[ML] Single Metric Viewer: Fix time bounds with custom strings. (#55045)
Browse files Browse the repository at this point in the history
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 committed Jan 17, 2020
1 parent 3e46060 commit f13adfa
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 f13adfa

Please sign in to comment.