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.

Sign up for GitHub

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

[Discover] Use summary column service name component for service name… #196742

Merged
merged 6 commits into from
Oct 22, 2024
Merged

[Discover] Use summary column service name component for service name… #196742

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c0b5e0
[Discover] Use summary column service name component for service name…
mohamedhamed-ahmed Oct 17, 2024
a7739b0
Fix Tests
mohamedhamed-ahmed Oct 20, 2024
6f1985b
Merge branch 'main' into 196541-replace-service-name-column-renderer
mohamedhamed-ahmed Oct 20, 2024
f07d2ac
Fix Agent Icon Style
mohamedhamed-ahmed Oct 21, 2024
f2d1f83
Merge branch 'main' into 196541-replace-service-name-column-renderer
mohamedhamed-ahmed Oct 21, 2024
530b4d1
Refactor Code
mohamedhamed-ahmed Oct 22, 2024
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
51 changes: 36 additions & 15 deletions src/plugins/discover/public/components/data_types/logs/service_name_cell.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,46 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import React from 'react';
import { buildDataTableRecord, DataTableRecord } from '@kbn/discover-utils';
import { dataViewMock } from '@kbn/discover-utils/src/__mocks__';
import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks';
import { render, screen } from '@testing-library/react';
import React from 'react';
import { DataGridDensity, ROWS_HEIGHT_OPTIONS } from '@kbn/unified-data-table';
import { getServiceNameCell } from './service_name_cell';
import { CellRenderersExtensionParams } from '../../../context_awareness';

const core = {
application: {
capabilities: {
apm: {
show: true,
},
},
},
uiSettings: {
get: () => true,
},
};

jest.mock('../../../hooks/use_discover_services', () => {
const originalModule = jest.requireActual('../../../hooks/use_discover_services');
return {
...originalModule,
useDiscoverServices: () => ({ core, share: {} }),
};
});

const renderCell = (serviceNameField: string, record: DataTableRecord) => {
const ServiceNameCell = getServiceNameCell(serviceNameField);
const cellRenderersExtensionParamsMock: CellRenderersExtensionParams = {
actions: {
addFilter: jest.fn(),
},
dataView: dataViewMock,
density: DataGridDensity.COMPACT,
rowHeight: ROWS_HEIGHT_OPTIONS.single,
};
const ServiceNameCell = getServiceNameCell(serviceNameField, cellRenderersExtensionParamsMock);
render(
<ServiceNameCell
rowIndex={0}
Expand All @@ -40,22 +71,12 @@ describe('getServiceNameCell', () => {
dataViewMock
);
renderCell('service.name', record);
expect(screen.getByTestId('serviceNameCell-nodejs')).toBeInTheDocument();
});

it('renders default icon with unknwon test subject if agent name is missing', () => {
const record = buildDataTableRecord(
{ fields: { 'service.name': 'test-service' } },
dataViewMock
);
renderCell('service.name', record);
expect(screen.getByTestId('serviceNameCell-unknown')).toBeInTheDocument();
expect(screen.getByTestId('dataTableCellActionsPopover_service.name')).toBeInTheDocument();
});

it('does not render if service name is missing', () => {
it('does render empty div if service name is missing', () => {
const record = buildDataTableRecord({ fields: { 'agent.name': 'nodejs' } }, dataViewMock);
renderCell('service.name', record);
expect(screen.queryByTestId('serviceNameCell-nodejs')).not.toBeInTheDocument();
expect(screen.queryByTestId('serviceNameCell-unknown')).not.toBeInTheDocument();
expect(screen.queryByTestId('serviceNameCell-empty')).toBeInTheDocument();
});
});
42 changes: 26 additions & 16 deletions src/plugins/discover/public/components/data_types/logs/service_name_cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,39 +7,49 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { EuiFlexGroup, EuiFlexItem, EuiToolTip } from '@elastic/eui';
import React from 'react';
import { EuiToolTip } from '@elastic/eui';
import type { AgentName } from '@kbn/elastic-agent-utils';
import { dynamic } from '@kbn/shared-ux-utility';
import type { DataGridCellValueElementProps } from '@kbn/unified-data-table';
import React from 'react';
import { css } from '@emotion/react';
import { getFieldValue } from '@kbn/discover-utils';
import { euiThemeVars } from '@kbn/ui-theme';
import { CellRenderersExtensionParams } from '../../../context_awareness';
import { AGENT_NAME_FIELD } from '../../../../common/data_types/logs/constants';
import { ServiceNameBadgeWithActions } from './service_name_badge_with_actions';

const dataTestSubj = 'serviceNameCell';
const AgentIcon = dynamic(() => import('@kbn/custom-icons/src/components/agent_icon'));

export const getServiceNameCell =
(serviceNameField: string) => (props: DataGridCellValueElementProps) => {
(serviceNameField: string, { actions }: CellRenderersExtensionParams) =>
(props: DataGridCellValueElementProps) => {
const serviceNameValue = getFieldValue(props.row, serviceNameField) as string;
const agentName = getFieldValue(props.row, AGENT_NAME_FIELD) as AgentName;

if (!serviceNameValue) {
return <span data-test-subj={`${dataTestSubj}-empty`}>-</span>;
}

const getIcon = () => (
<EuiToolTip position="left" content={agentName} repositionOnScroll={true}>
<AgentIcon
agentName={agentName}
size="m"
css={css`
margin-right: ${euiThemeVars.euiSizeXS};
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this but also see the icon bring right up against the text. Is this what this line is aiming to avoid or do we need to add margin here too? (see image)
CleanShot 2024-10-21 at 07 30 35@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryankeairns is the image taken from the video? because I guess the video was before the fix of applying the styles

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: since these styles are the same across instances, declaring them as a constant outside of the component would be more performant. I usually wouldn't recommend such a micro optimization, but it's worth considering since performance can be very sensitive within grid cells where we render so many at once.

`}
/>
</EuiToolTip>
);

return (
<EuiFlexGroup
gutterSize="xs"
data-test-subj={`${dataTestSubj}-${agentName || 'unknown'}`}
responsive={false}
alignItems="center"
>
<EuiFlexItem grow={false}>
<EuiToolTip position="left" content={agentName} repositionOnScroll={true}>
<AgentIcon agentName={agentName} size="m" />
</EuiToolTip>
</EuiFlexItem>
<EuiFlexItem grow={false}>{serviceNameValue}</EuiFlexItem>
</EuiFlexGroup>
<ServiceNameBadgeWithActions
onFilter={actions.addFilter}
icon={getIcon}
value={serviceNameValue}
property={serviceNameField}
/>
);
};
4 changes: 2 additions & 2 deletions ...reness/profile_providers/common/logs_data_source_profile/accessors/get_cell_renderers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ export const getCellRenderers: DataSourceProfileProvider['profile']['getCellRend
...SERVICE_NAME_FIELDS.reduce(
(acc, field) => ({
...acc,
[field]: getServiceNameCell(field),
[`${field}.keyword`]: getServiceNameCell(`${field}.keyword`),
[field]: getServiceNameCell(field, params),
[`${field}.keyword`]: getServiceNameCell(`${field}.keyword`, params),
}),
{}
),
Expand Down
20 changes: 14 additions & 6 deletions test/functional/apps/discover/context_awareness/extensions/_get_cell_renderers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

const firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 0);
const lastCell = await dataGrid.getCellElementExcludingControlColumns(2, 0);
const firstServiceNameCell = await firstCell.findByTestSubject('serviceNameCell-java');
const lastServiceNameCell = await lastCell.findByTestSubject('serviceNameCell-unknown');
const firstServiceNameCell = await firstCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
const lastServiceNameCell = await lastCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
expect(await firstServiceNameCell.getVisibleText()).to.be('product');
expect(await lastServiceNameCell.getVisibleText()).to.be('accounting');
});
Expand All @@ -130,7 +134,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await retry.try(async () => {
const firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 0);
expect(await firstCell.getVisibleText()).to.be('product');
await testSubjects.missingOrFail('*serviceNameCell*');
await testSubjects.missingOrFail('dataTableCellActionsPopover_service.name');
});
});
});
Expand Down Expand Up @@ -278,8 +282,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await retry.try(async () => {
firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 1);
lastCell = await dataGrid.getCellElementExcludingControlColumns(2, 1);
const firstServiceNameCell = await firstCell.findByTestSubject('serviceNameCell-java');
const lastServiceNameCell = await lastCell.findByTestSubject('serviceNameCell-unknown');
const firstServiceNameCell = await firstCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
const lastServiceNameCell = await lastCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
expect(await firstServiceNameCell.getVisibleText()).to.be('product');
expect(await lastServiceNameCell.getVisibleText()).to.be('accounting');
});
Expand Down Expand Up @@ -309,7 +317,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

expect(await firstCell.getVisibleText()).to.be('product');
expect(await lastCell.getVisibleText()).to.be('accounting');
await testSubjects.missingOrFail('*serviceNameCell*');
await testSubjects.missingOrFail('dataTableCellActionsPopover_service.name');
});
});
});
Expand Down
20 changes: 14 additions & 6 deletions ...unctional/test_suites/common/discover/context_awareness/extensions/_get_cell_renderers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

const firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 0);
const lastCell = await dataGrid.getCellElementExcludingControlColumns(2, 0);
const firstServiceNameCell = await firstCell.findByTestSubject('serviceNameCell-java');
const lastServiceNameCell = await lastCell.findByTestSubject('serviceNameCell-unknown');
const firstServiceNameCell = await firstCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
const lastServiceNameCell = await lastCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
expect(await firstServiceNameCell.getVisibleText()).to.be('product');
expect(await lastServiceNameCell.getVisibleText()).to.be('accounting');
});
Expand All @@ -130,7 +134,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await retry.try(async () => {
const firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 0);
expect(await firstCell.getVisibleText()).to.be('product');
await testSubjects.missingOrFail('*serviceNameCell*');
await testSubjects.missingOrFail('dataTableCellActionsPopover_service.name');
});
});
});
Expand Down Expand Up @@ -277,8 +281,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await retry.try(async () => {
firstCell = await dataGrid.getCellElementExcludingControlColumns(0, 1);
lastCell = await dataGrid.getCellElementExcludingControlColumns(2, 1);
const firstServiceNameCell = await firstCell.findByTestSubject('serviceNameCell-java');
const lastServiceNameCell = await lastCell.findByTestSubject('serviceNameCell-unknown');
const firstServiceNameCell = await firstCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
const lastServiceNameCell = await lastCell.findByTestSubject(
'dataTableCellActionsPopover_service.name'
);
expect(await firstServiceNameCell.getVisibleText()).to.be('product');
expect(await lastServiceNameCell.getVisibleText()).to.be('accounting');
});
Expand Down Expand Up @@ -308,7 +316,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

expect(await firstCell.getVisibleText()).to.be('product');
expect(await lastCell.getVisibleText()).to.be('accounting');
await testSubjects.missingOrFail('*serviceNameCell*');
await testSubjects.missingOrFail('dataTableCellActionsPopover_service.name');
});
});
});
Expand Down