Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[App Search] API Logs: Add ApiLogsTable and NewApiEventsPrompt components #96008

Merged
merged 7 commits into from
Apr 2, 2021
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
Expand Up @@ -17,6 +17,8 @@ import { EuiPageHeader } from '@elastic/eui';
import { Loading } from '../../../shared/loading';
import { LogRetentionCallout, LogRetentionTooltip } from '../log_retention';

import { ApiLogsTable, NewApiEventsPrompt } from './components';

import { ApiLogs } from './';

describe('ApiLogs', () => {
Expand All @@ -41,7 +43,8 @@ describe('ApiLogs', () => {

it('renders', () => {
expect(wrapper.find(EuiPageHeader).prop('pageTitle')).toEqual('API Logs');
// TODO: Check for ApiLogsTable + NewApiEventsPrompt when those get added
expect(wrapper.find(ApiLogsTable)).toHaveLength(1);
expect(wrapper.find(NewApiEventsPrompt)).toHaveLength(1);

expect(wrapper.find(LogRetentionCallout).prop('type')).toEqual('api');
expect(wrapper.find(LogRetentionTooltip).prop('type')).toEqual('api');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@ import React, { useEffect } from 'react';

import { useValues, useActions } from 'kea';

import { EuiPageHeader, EuiTitle, EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
import {
EuiPageHeader,
EuiTitle,
EuiPageContent,
EuiPageContentBody,
EuiFlexGroup,
EuiFlexItem,
EuiSpacer,
} from '@elastic/eui';

import { FlashMessages } from '../../../shared/flash_messages';
import { SetAppSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome';
Expand All @@ -18,6 +26,7 @@ import { Loading } from '../../../shared/loading';

import { LogRetentionCallout, LogRetentionTooltip, LogRetentionOptions } from '../log_retention';

import { ApiLogsTable, NewApiEventsPrompt } from './components';
import { API_LOGS_TITLE, RECENT_API_EVENTS } from './constants';

import { ApiLogsLogic } from './';
Expand Down Expand Up @@ -47,19 +56,27 @@ export const ApiLogs: React.FC<Props> = ({ engineBreadcrumb }) => {
<FlashMessages />
<LogRetentionCallout type={LogRetentionOptions.API} />

<EuiFlexGroup gutterSize="m" alignItems="center" responsive={false} wrap>
<EuiFlexItem grow={false}>
<EuiTitle size="s">
<h2>{RECENT_API_EVENTS}</h2>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<LogRetentionTooltip type={LogRetentionOptions.API} />
</EuiFlexItem>
<EuiFlexItem grow={false}>{/* TODO: NewApiEventsPrompt */}</EuiFlexItem>
</EuiFlexGroup>
<EuiPageContent hasBorder>
<EuiPageContentBody>
<EuiFlexGroup gutterSize="m" alignItems="center" responsive={false} wrap>
Comment on lines +59 to +61
Copy link
Member Author

@cee-chen cee-chen Mar 31, 2021

Choose a reason for hiding this comment

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

Everything inside here is mostly the same as before, I just added EuipageContent and EuiPageContentBody wrappers - I recommend viewing with hidden whitespace changes for easier diffing

<EuiFlexItem grow={false}>
<EuiTitle size="s">
<h2>{RECENT_API_EVENTS}</h2>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<LogRetentionTooltip type={LogRetentionOptions.API} />
</EuiFlexItem>
<EuiFlexItem />
<EuiFlexItem grow={false}>
<NewApiEventsPrompt />
</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer size="m" />

{/* TODO: ApiLogsTable */}
<ApiLogsTable hasPagination />
</EuiPageContentBody>
</EuiPageContent>
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.apiLogDetailButton {
// More closely mimics the regular line height of an EuiLink /
// compresses table rows back to the standard height
height: $euiSizeL !important;
Copy link
Member Author

Choose a reason for hiding this comment

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

With:

Without:

I was trying to maintain the more compact rows from our standalone UI (as well as matching our other tables), but if we think that's overkill I can get rid of it.

FWIW without the ! important I need 3 levels of nesting to override EUI's styles, so I took the lazy way out 😬

Copy link
Member

Choose a reason for hiding this comment

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

If it were me, I'd say just let EUI do it's thing. However, if there's something unique about this table that is forcing the rows to be taller than usual, then I'd say this is a worthwhile change.

It's up to you though. Is it worthwhile to drop a comment in the CSS about why you have added that or nah?

To be clear, I'm good either way, I'll approve it regardless.

Copy link
Member Author

@cee-chen cee-chen Apr 1, 2021

Choose a reason for hiding this comment

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

The first screenshot more closely matches the row heights in all their examples in their docs. The main difference is that their buttons are all icon buttons and not empty buttons 🤷 It does seem odd that EUI wouldn't have any affordance OOTB for EuiEmptyButton matching the same line height as an EuiLink.

A comment makes a ton of sense! I'll add one here in a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { setMockValues, setMockActions, mountWithIntl } from '../../../../__mocks__';

// NOTE: We're mocking FormattedRelative here because it (currently) has
// console warn issues, and it allows us to skip mocking dates
jest.mock('@kbn/i18n/react', () => ({
...(jest.requireActual('@kbn/i18n/react') as object),
FormattedRelative: jest.fn(() => '20 hours ago'),
}));

import React from 'react';

import { shallow } from 'enzyme';

import { EuiBasicTable, EuiBadge, EuiHealth, EuiButtonEmpty, EuiEmptyPrompt } from '@elastic/eui';

import { DEFAULT_META } from '../../../../shared/constants';

import { ApiLogsTable } from './';

describe('ApiLogsTable', () => {
const apiLogs = [
{
timestamp: '1970-01-01T00:00:00.000Z',
status: 404,
http_method: 'GET',
full_request_path: '/api/as/v1/test',
},
{
timestamp: '1970-01-01T00:00:00.000Z',
status: 500,
http_method: 'DELETE',
full_request_path: '/api/as/v1/test',
},
{
timestamp: '1970-01-01T00:00:00.000Z',
status: 200,
http_method: 'POST',
full_request_path: '/api/as/v1/engines/some-engine/search',
},
];

const values = {
dataLoading: false,
apiLogs,
meta: DEFAULT_META,
};
const actions = {
onPaginate: jest.fn(),
};

beforeEach(() => {
jest.clearAllMocks();
setMockValues(values);
setMockActions(actions);
});

it('renders', () => {
const wrapper = mountWithIntl(<ApiLogsTable />);
const tableContent = wrapper.find(EuiBasicTable).text();

expect(tableContent).toContain('Method');
expect(tableContent).toContain('GET');
expect(tableContent).toContain('DELETE');
expect(tableContent).toContain('POST');
expect(wrapper.find(EuiBadge)).toHaveLength(3);

expect(tableContent).toContain('Time');
expect(tableContent).toContain('20 hours ago');

expect(tableContent).toContain('Endpoint');
expect(tableContent).toContain('/api/as/v1/test');
expect(tableContent).toContain('/api/as/v1/engines/some-engine/search');

expect(tableContent).toContain('Status');
expect(tableContent).toContain('404');
expect(tableContent).toContain('500');
expect(tableContent).toContain('200');
expect(wrapper.find(EuiHealth)).toHaveLength(3);

expect(wrapper.find(EuiButtonEmpty)).toHaveLength(3);
wrapper.find('[data-test-subj="ApiLogsTableDetailsButton"]').first().simulate('click');
// TODO: API log details flyout
Comment on lines +88 to +89
Copy link
Member

Choose a reason for hiding this comment

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

So it looks like you intend to test the flyout opening in the primary renders test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, just realized I forgot to respond to this - sorry Jason! Yeah the tables are a bit weird testing-wise and mount is a somewhat-expensive render so I figured just throw it in the main test. I can move it out though if we prefer

});

it('renders an empty prompt if no items are passed', () => {
setMockValues({ ...values, apiLogs: [] });
const wrapper = mountWithIntl(<ApiLogsTable />);
const promptContent = wrapper.find(EuiEmptyPrompt).text();

expect(promptContent).toContain('No recent logs');
});

describe('hasPagination', () => {
it('does not render with pagination by default', () => {
const wrapper = shallow(<ApiLogsTable />);

expect(wrapper.find(EuiBasicTable).prop('pagination')).toBeFalsy();
});

it('renders pagination if hasPagination is true', () => {
const wrapper = shallow(<ApiLogsTable hasPagination />);

expect(wrapper.find(EuiBasicTable).prop('pagination')).toBeTruthy();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import { useValues, useActions } from 'kea';

import {
EuiBasicTable,
EuiBasicTableColumn,
EuiBadge,
EuiHealth,
EuiButtonEmpty,
EuiEmptyPrompt,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedRelative } from '@kbn/i18n/react';

import { convertMetaToPagination, handlePageChange } from '../../../../shared/table_pagination';

import { ApiLogsLogic } from '../index';
import { ApiLog } from '../types';
import { getStatusColor } from '../utils';

import './api_logs_table.scss';

interface Props {
hasPagination?: boolean;
}
export const ApiLogsTable: React.FC<Props> = ({ hasPagination }) => {
const { dataLoading, apiLogs, meta } = useValues(ApiLogsLogic);
const { onPaginate } = useActions(ApiLogsLogic);

const columns: Array<EuiBasicTableColumn<ApiLog>> = [
{
field: 'http_method',
name: i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.methodTableHeading', {
defaultMessage: 'Method',
}),
width: '100px',
render: (method: string) => <EuiBadge color="primary">{method}</EuiBadge>,
},
{
field: 'timestamp',
name: i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.timeTableHeading', {
defaultMessage: 'Time',
}),
width: '20%',
render: (dateString: string) => <FormattedRelative value={new Date(dateString)} />,
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
},
{
field: 'full_request_path',
name: i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.endpointTableHeading', {
defaultMessage: 'Endpoint',
}),
width: '50%',
truncateText: true,
mobileOptions: {
// @ts-ignore - EUI's typing is incorrect here
width: '100%',
},
},
{
field: 'status',
name: i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.statusTableHeading', {
defaultMessage: 'Status',
}),
dataType: 'number',
width: '100px',
render: (status: number) => <EuiHealth color={getStatusColor(status)}>{status}</EuiHealth>,
},
{
width: '100px',
align: 'right',
render: (apiLog: ApiLog) => (
<EuiButtonEmpty
size="s"
className="apiLogDetailButton"
data-test-subj="ApiLogsTableDetailsButton"
// TODO: flyout onclick
>
{i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.detailsButtonLabel', {
defaultMessage: 'Details',
})}
</EuiButtonEmpty>
),
},
];

const paginationProps = hasPagination
? {
pagination: {
...convertMetaToPagination(meta),
hidePerPageOptions: true,
},
onChange: handlePageChange(onPaginate),
}
: {};

return (
<EuiBasicTable
columns={columns}
items={apiLogs}
responsive
loading={dataLoading}
noItemsMessage={
<EuiEmptyPrompt
iconType="clock"
title={
<h3>
{i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.emptyTitle', {
defaultMessage: 'No recent logs',
})}
</h3>
}
body={
<p>
{i18n.translate('xpack.enterpriseSearch.appSearch.engine.apiLogs.emptyDescription', {
defaultMessage: "Check back after you've performed some API calls.",
})}
</p>
}
/>
}
{...paginationProps}
/>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export { ApiLogsTable } from './api_logs_table';
export { NewApiEventsPrompt } from './new_api_events_prompt';
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.newApiEventsPrompt {
padding: $euiSizeXS;
padding-left: $euiSizeS;
Comment on lines +2 to +3
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: without this custom padding, the height of the panel exceeds the height of the row height and causes the page to bounce around awkwardly, so I added it as a polish item

display: flex;
align-items: center;
}
Loading