Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/*
* 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 { resetContext } from 'kea';

import { mockHttpValues } from '../../../__mocks__';
jest.mock('../../../shared/http', () => ({
HttpLogic: { values: mockHttpValues },
}));
const { http } = mockHttpValues;

jest.mock('../../../shared/flash_messages', () => ({
flashAPIErrors: jest.fn(),
}));
import { flashAPIErrors } from '../../../shared/flash_messages';

jest.mock('../engine', () => ({
Comment on lines +10 to +20
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really like to figure out someday how to DRY out all these mocks to a reusable helper, especially for the shared ones, but I'm not sure we'll ever be able to because of the relative ../ paths.

Maybe someday we can ask the Kibana team if absolute webpack alias's are possible for xpack plugins - we can but dream!

EngineLogic: { values: { engineName: 'some-engine' } },
}));

import { EngineOverviewLogic } from './';

describe('EngineOverviewLogic', () => {
const mockEngineMetrics = {
apiLogsUnavailable: true,
documentCount: 10,
startDate: '1970-01-30',
endDate: '1970-01-31',
operationsPerDay: [0, 0, 0, 0, 0, 0, 0],
queriesPerDay: [0, 0, 0, 0, 0, 25, 50],
totalClicks: 50,
totalQueries: 75,
};

const DEFAULT_VALUES = {
dataLoading: true,
apiLogsUnavailable: false,
documentCount: 0,
startDate: '',
endDate: '',
operationsPerDay: [],
queriesPerDay: [],
totalClicks: 0,
totalQueries: 0,
timeoutId: null,
};

const mount = () => {
resetContext({});
EngineOverviewLogic.mount();
};

beforeEach(() => {
jest.clearAllMocks();
});

it('has expected default values', () => {
mount();
expect(EngineOverviewLogic.values).toEqual(DEFAULT_VALUES);
});

describe('actions', () => {
describe('setPolledData', () => {
it('should set all received data as top-level values and set dataLoading to false', () => {
mount();
EngineOverviewLogic.actions.setPolledData(mockEngineMetrics);

expect(EngineOverviewLogic.values).toEqual({
...DEFAULT_VALUES,
...mockEngineMetrics,
dataLoading: false,
});
});
});

describe('setTimeoutId', () => {
describe('timeoutId', () => {
it('should be set to the provided value', () => {
mount();
EngineOverviewLogic.actions.setTimeoutId(123);

expect(EngineOverviewLogic.values).toEqual({
...DEFAULT_VALUES,
timeoutId: 123,
});
});
});
});

describe('pollForOverviewMetrics', () => {
it('fetches data and calls onPollingSuccess', async () => {
mount();
jest.spyOn(EngineOverviewLogic.actions, 'onPollingSuccess');
const promise = Promise.resolve(mockEngineMetrics);
http.get.mockReturnValueOnce(promise);

EngineOverviewLogic.actions.pollForOverviewMetrics();
await promise;

expect(http.get).toHaveBeenCalledWith('/api/app_search/engines/some-engine/overview');
expect(EngineOverviewLogic.actions.onPollingSuccess).toHaveBeenCalledWith(
mockEngineMetrics
);
});

it('handles errors', async () => {
mount();
const promise = Promise.reject('An error occurred');
http.get.mockReturnValue(promise);

try {
EngineOverviewLogic.actions.pollForOverviewMetrics();
await promise;
} catch {
// Do nothing
}
expect(flashAPIErrors).toHaveBeenCalledWith('An error occurred');
});
});

describe('onPollingSuccess', () => {
it('starts a polling timeout and sets data', async () => {
mount();
jest.useFakeTimers();
jest.spyOn(EngineOverviewLogic.actions, 'setTimeoutId');
jest.spyOn(EngineOverviewLogic.actions, 'setPolledData');

EngineOverviewLogic.actions.onPollingSuccess(mockEngineMetrics);

expect(setTimeout).toHaveBeenCalledWith(
EngineOverviewLogic.actions.pollForOverviewMetrics,
5000
);
expect(EngineOverviewLogic.actions.setTimeoutId).toHaveBeenCalledWith(expect.any(Number));
expect(EngineOverviewLogic.actions.setPolledData).toHaveBeenCalledWith(mockEngineMetrics);
});
});
});

describe('unmount', () => {
let unmount: Function;

beforeEach(() => {
jest.useFakeTimers();
resetContext({});
unmount = EngineOverviewLogic.mount();
});

it('clears existing polling timeouts on unmount', () => {
EngineOverviewLogic.actions.setTimeoutId(123);
unmount();
expect(clearTimeout).toHaveBeenCalled();
});

it("does not clear timeout if one hasn't been set", () => {
unmount();
expect(clearTimeout).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* 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 { kea, MakeLogicType } from 'kea';

import { flashAPIErrors } from '../../../shared/flash_messages';
import { HttpLogic } from '../../../shared/http';
import { EngineLogic } from '../engine';

const POLLING_DURATION = 5000;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per a convo in the front-end channel, I think we're just gonna set per-view POLLING_DURATION timing instead of trying to share a single const. We can come back later and look at this if this changes for any reason


interface EngineOverviewApiData {
apiLogsUnavailable: boolean;
documentCount: number;
startDate: string;
endDate: string;
operationsPerDay: number[];
queriesPerDay: number[];
totalClicks: number;
totalQueries: number;
}
interface EngineOverviewValues extends EngineOverviewApiData {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - this drops/cleans up a good amount of values from ent-search:

  • apiKey & apiUrl - this wasn't being used anywhere, not actually sure where it was coming from (maybe leftover from old logic). apiKey should just be grabbed directly from EngineLogic.values.engine.apiKey in any case.

  • accountKey - no longer applies in a SMAS world

  • role - also not used anywhere AFAICT, should just be pulled directly from AppLogic.values.myRole

  • pollingCancelTokenSource - per a discussion w/ Byron, we will no longer be using/storing axios cancel tokens

  • showDocumentCreationModal & creationMode - I'm going to be spiking out something in a few PRs here where DocumentCreationModal handles its own show/hide state and creation mode state instead of parent views doing it in multiple places.

  • activeApiLog - Same as the above, I believe ApiLogTables should be in charge of managing the state of its details modal, not the parent. I'm removing it for now on that premise, but if for any reason my belief is incorrect we can come back in later and add it

dataLoading: boolean;
timeoutId: number | null;
}

interface EngineOverviewActions {
setPolledData(engineMetrics: EngineOverviewApiData): EngineOverviewApiData;
setTimeoutId(timeoutId: number): { timeoutId: number };
pollForOverviewMetrics(): void;
onPollingSuccess(engineMetrics: EngineOverviewApiData): EngineOverviewApiData;
}

export const EngineOverviewLogic = kea<MakeLogicType<EngineOverviewValues, EngineOverviewActions>>({
path: ['enterprise_search', 'app_search', 'engine_overview_logic'],
actions: () => ({
setPolledData: (engineMetrics) => engineMetrics,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking feedback. But it usually only takes a moment to do, so when I convert these logic files I often change actions to return objects based on Kea's recommendation:

One more thing. It's strongly recommended to always return an object as a payload from your actions:

Suggested change
setPolledData: (engineMetrics) => engineMetrics,
setPolledData: (engineMetrics) => { engineMetrics },

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is super bizarre - I thought I left a comment on this but it appears to have disappeared :| So unfortunately based on the way the logic file is written we actually can't do this here without significantly rewriting / adding more destructuring for every single top-level value in the file. I haven't seen any other logic file do this, but in this one, we pull almost every top-level key out of the API response and set it as its own value (see the test).

Which means if we passed engineMetrics as an obj key, we now have to destructure that and do something like:

         setPolledData: (_, { engineMetrics: { apiLogsUnavailable } }) => apiLogsUnavailable,

Which isn't the worst, but I went back and forth on this and ended up just deciding to leave it as it was from the current code. I definitely think this page and logic file could stand to be looked at first from a refactor POV in the future, as I believe it's one of our earliest/oldest views.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

setTimeoutId: (timeoutId) => ({ timeoutId }),
pollForOverviewMetrics: true,
onPollingSuccess: (engineMetrics) => engineMetrics,
}),
reducers: () => ({
dataLoading: [
true,
{
setPolledData: () => false,
},
],
apiLogsUnavailable: [
false,
{
setPolledData: (_, { apiLogsUnavailable }) => apiLogsUnavailable,
},
],
startDate: [
'',
{
setPolledData: (_, { startDate }) => startDate,
},
],
endDate: [
'',
{
setPolledData: (_, { endDate }) => endDate,
},
],
queriesPerDay: [
[],
{
setPolledData: (_, { queriesPerDay }) => queriesPerDay,
},
],
operationsPerDay: [
[],
{
setPolledData: (_, { operationsPerDay }) => operationsPerDay,
},
],
totalQueries: [
0,
{
setPolledData: (_, { totalQueries }) => totalQueries,
},
],
totalClicks: [
0,
{
setPolledData: (_, { totalClicks }) => totalClicks,
},
],
documentCount: [
0,
{
setPolledData: (_, { documentCount }) => documentCount,
},
],
timeoutId: [
null,
{
setTimeoutId: (_, { timeoutId }) => timeoutId,
},
],
}),
listeners: ({ actions }) => ({
pollForOverviewMetrics: async () => {
const { http } = HttpLogic.values;
const { engineName } = EngineLogic.values;

try {
const response = await http.get(`/api/app_search/engines/${engineName}/overview`);
actions.onPollingSuccess(response);
} catch (e) {
flashAPIErrors(e);
}
},
onPollingSuccess: (engineMetrics) => {
const timeoutId = window.setTimeout(actions.pollForOverviewMetrics, POLLING_DURATION);
actions.setTimeoutId(timeoutId);
Copy link
Copy Markdown
Member

@JasonStoltz JasonStoltz Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change this, this is non-blocking feedback. Just wanted to point out a best practice from Kea and Redux.

https://kea.js.org/docs/guide/concepts#listeners

If you read the reducers section above, you'll remember that it's an anti-pattern to only have setThis and setThat actions that only update this or that.

Often times when you need to call 2 or more actions like this on success to set resulting data, it's a good indication that you're following this anti-pattern.

There really is only 1 "action" happening here. Meaning, if you think of actions as "events" like Kea suggestion, the "event" that is happening here is that polling has completed. Reducers should react to that "event" and set data accordingly.

I could be mis-reading this, just thought I'd take a moment to point this out.

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh super interesting! I went back and forth a bit on whether I should refactor/clean this up as well actually but ended up leaving it as-is because it didn't bother me too much.

If you read the reducers section above, you'll remember that it's an anti-pattern to only have setThis and setThat actions that only update this or that.

This confuses me a bit, we do this in a number of our Kea files and I'm not quite clear on what the "correct" code looks like (maybe just a bit too early in the morning for me).

I did just discover sharedListeners in their docs which blew my mind (how long has that been there!?) and looks like we could use it in our code... I'd love to do an overall Kea refactor at some point if we have time after all this is over 😅

actions.setPolledData(engineMetrics);
},
}),
events: ({ values }) => ({
beforeUnmount() {
if (values.timeoutId !== null) clearTimeout(values.timeoutId);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw a post about nullish operators/assignment on Twitter recently and almost kinda wanted to try it out here:

Suggested change
if (values.timeoutId !== null) clearTimeout(values.timeoutId);
values.timeoutId ?? clearTimeout(values.timeoutId);

But it felt kinda weird to read and Typescript didn't like it. haha

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why TS didn't like it?

One interesting thing...

In the former, if values.timeoutId is undefined, clearTimeout would be called.

In the later, if values.timeoutId is undefined, clearTimeout would NOT be called.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code now, I realize that is irrelevant, because timeoutId would never be null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS probably isn't yet fully updated for nullish operators is my guess.

timeoutId would never be null

I think you meant undefined here? If so, correct, a redux value can never be undefined or the entire app just crashes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's often a large disconnect between my brain and my fingers 😬

},
}),
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* 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.
*/

export { EngineOverviewLogic } from './engine_overview_logic';
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ describe('engine routes', () => {
mockRouter = new MockRouter({
method: 'get',
path: '/api/app_search/engines/{name}',
payload: 'params',
});

registerEnginesRoutes({
Expand All @@ -133,4 +132,29 @@ describe('engine routes', () => {
});
});
});

describe('GET /api/app_search/engines/{name}/overview', () => {
let mockRouter: MockRouter;

beforeEach(() => {
jest.clearAllMocks();
mockRouter = new MockRouter({
method: 'get',
path: '/api/app_search/engines/{name}/overview',
});

registerEnginesRoutes({
...mockDependencies,
router: mockRouter.router,
});
});

it('creates a request to enterprise search', () => {
mockRouter.callRoute({ params: { name: 'some-engine' } });

expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/overview_metrics',
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,19 @@ export function registerEnginesRoutes({
})(context, request, response);
}
);
router.get(
{
path: '/api/app_search/engines/{name}/overview',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this before, I'll mention it again to see what you think.

I don't particularly like renaming routes. If this forwards to /overview_metrics in Enterprise Search, I think it should also be /overview_metrics in the Kibana API.

It just simplifies things, IMO. One less thing to think about. If you see /overview_metrics throwing an error, it's simpler to just go look for the /overview_metrics API endpoint in the backend to debug, rather than first checking the server mapping to see what the real API endpoint is on the backend to debug.

That's my totally subjective opinion. Feel free to disregard and keep it as is. I don't think it ultimately will make a big difference one way or the other.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a super fair point - what I was thinking originally when looking at this endpoint / view was:

  1. What guarantees that our Overview page will only ever show metrics in the future? Why not show/pass other non-metric information?
  2. If we ever do at that point, we would (likely?) change the name of the endpoint, and a generic /overview (to match the page) is more future-proof

That being said, I'm hoping we take a second pass at all our API endpoints and where possible convert internal ones to standard API endpoints in some distant future - at that point it would be great to look at names as well.

validate: {
params: schema.object({
name: schema.string(),
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.name}/overview_metrics`,
})(context, request, response);
}
);
}