-
Notifications
You must be signed in to change notification settings - Fork 61
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
Traces - Custom source switch to data grid #2390
base: main
Are you sure you want to change the base?
Changes from 1 commit
f059ffa
599145b
46bbd01
21378f7
8e64bf7
1e55ae1
d35a515
0492210
5e42e11
5c5090a
e54b7a3
d64b67d
b3adb25
d37c2b5
5e89941
8ed7260
f5ff592
5974927
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Adam Tackett <tackadam@amazon.com>
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,7 +8,7 @@ | |||||
import { EuiEmptyPrompt, EuiSmallButtonEmpty, EuiSpacer, EuiText } from '@elastic/eui'; | ||||||
import { SpacerSize } from '@elastic/eui/src/components/spacer/spacer'; | ||||||
import { isEmpty, round } from 'lodash'; | ||||||
import React from 'react'; | ||||||
import React, { useEffect } from 'react'; | ||||||
import { | ||||||
DATA_PREPPER_INDEX_NAME, | ||||||
DATA_PREPPER_SERVICE_INDEX_NAME, | ||||||
|
@@ -252,11 +252,11 @@ | |||||
|
||||||
const averageLatency = `Average duration: ${ | ||||||
map[service].latency! >= 0 ? map[service].latency + 'ms' : '-' | ||||||
}`; | ||||||
}`; | ||||||
|
||||||
const errorRate = `Error rate: ${ | ||||||
map[service].error_rate! >= 0 ? map[service].error_rate + '%' : '-' | ||||||
}`; | ||||||
}`; | ||||||
|
||||||
const throughput = | ||||||
'Request rate: ' + | ||||||
|
@@ -360,7 +360,7 @@ | |||||
percentileMaps: Array<{ traceGroupName: string; durationFilter: { gte?: number; lte?: number } }>, | ||||||
conditionString: string // >= 95th, < 95th | ||||||
): FilterType => { | ||||||
const DSL: any = { | ||||||
query: { | ||||||
bool: { | ||||||
must: [], | ||||||
|
@@ -405,12 +405,12 @@ | |||||
mode: TraceAnalyticsMode, | ||||||
filters: FilterType[], | ||||||
query: string, | ||||||
startTime: any, | ||||||
endTime: any, | ||||||
page?: string, | ||||||
appConfigs: FilterType[] = [] | ||||||
) => { | ||||||
const DSL: any = { | ||||||
query: { | ||||||
bool: { | ||||||
must: [], | ||||||
|
@@ -677,3 +677,83 @@ | |||||
return []; | ||||||
} | ||||||
}; | ||||||
|
||||||
export const InjectElementsIntoGrid = ( | ||||||
rowCount: number, | ||||||
maxDisplayRows: number, | ||||||
tracesTableMode: string, | ||||||
loadMoreHandler?: () => void | ||||||
) => { | ||||||
useEffect(() => { | ||||||
setTimeout(() => { | ||||||
const toolbar = document.querySelector('.euiDataGrid__controls') as HTMLElement; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. it's better to avoid
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed all as assertions. |
||||||
if (toolbar && rowCount > maxDisplayRows) { | ||||||
toolbar.style.display = 'flex'; | ||||||
toolbar.style.alignItems = 'center'; | ||||||
toolbar.style.justifyContent = 'space-between'; | ||||||
|
||||||
let warningDiv = toolbar.querySelector('.trace-table-warning') as HTMLElement; | ||||||
if (!warningDiv) { | ||||||
warningDiv = document.createElement('div'); | ||||||
warningDiv.className = 'trace-table-warning'; | ||||||
warningDiv.style.marginLeft = 'auto'; | ||||||
warningDiv.style.color = '#a87900'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this visible in both dark and light mode? if not oui might have some class names for coloring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
warningDiv.style.fontSize = '13px'; | ||||||
warningDiv.style.display = 'flex'; | ||||||
warningDiv.style.alignItems = 'center'; | ||||||
warningDiv.style.padding = '2px 8px'; | ||||||
warningDiv.style.borderRadius = '4px'; | ||||||
warningDiv.style.whiteSpace = 'nowrap'; | ||||||
|
||||||
const icon = document.createElement('span'); | ||||||
icon.className = 'euiIcon euiIcon--medium euiIcon--warning'; | ||||||
icon.setAttribute('aria-label', 'Warning'); | ||||||
icon.style.marginRight = '4px'; | ||||||
|
||||||
const strongElement = document.createElement('strong'); | ||||||
strongElement.textContent = `${maxDisplayRows}`; | ||||||
|
||||||
const textSpan = document.createElement('span'); | ||||||
textSpan.appendChild(strongElement); | ||||||
textSpan.appendChild(document.createTextNode(" results shown out of ")); | ||||||
textSpan.appendChild(document.createTextNode(` ${rowCount}`)); | ||||||
|
||||||
warningDiv.appendChild(icon); | ||||||
warningDiv.appendChild(textSpan); | ||||||
|
||||||
toolbar.appendChild(warningDiv); | ||||||
} | ||||||
} | ||||||
|
||||||
const pagination = document.querySelector('.euiDataGrid__pagination') as HTMLElement; | ||||||
|
||||||
if (tracesTableMode !== "traces") { | ||||||
if (pagination) { | ||||||
const existingLoadMoreButton = pagination.querySelector('.trace-table-load-more'); | ||||||
if (existingLoadMoreButton) { | ||||||
existingLoadMoreButton.remove(); | ||||||
} | ||||||
} | ||||||
return; | ||||||
} | ||||||
|
||||||
if (pagination && loadMoreHandler) { | ||||||
pagination.style.display = 'flex'; | ||||||
pagination.style.alignItems = 'center'; | ||||||
pagination.style.justifyContent = 'space-between'; | ||||||
|
||||||
let loadMoreButton = pagination.querySelector('.trace-table-load-more') as HTMLElement; | ||||||
if (!loadMoreButton) { | ||||||
loadMoreButton = document.createElement('button'); | ||||||
loadMoreButton.className = 'trace-table-load-more euiButtonEmpty euiButtonEmpty--text'; | ||||||
loadMoreButton.style.marginLeft = '12px'; | ||||||
loadMoreButton.innerText = 'Load more data'; | ||||||
|
||||||
loadMoreButton.onclick = () => loadMoreHandler(); | ||||||
|
||||||
pagination.appendChild(loadMoreButton); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DOM manipulation is hard to maintain and might break when OUI library changes. is it possible to use existing data grid functions to do this? for example https://oui.opensearch.org/1.19/#/tabular-content/data-grid-styling-and-control#additional-controls-in-the-toolbar There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, Data grid “Additional controls” only allows insertion to the left of the default controls. We are currently using that to add the Span/Trace selection and Fullscreen buttons. The component does not allow for buttons to be split into different locations to have the warning in the right corner. It also doesn’t allow for manipulation of the pagination to insert the “Load more” button at the bottom right. With current UX requirements, I think DOM manipulation is the best solution; pending a refactor / adjustments of the OUI component. |
||||||
} | ||||||
} | ||||||
}, 100); | ||||||
}, [rowCount, tracesTableMode, loadMoreHandler]); | ||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the helper_functions file is getting too lang, it might be better to split this into a different file especially it's a custom react hook
also it's better to name hooks with
use
prefix to help lintersand could you add some comments on top or make naming more specific, for example what elements are injected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made new file public/components/trace_analytics/components/common/shared_components/component_helper_functions.tsx
And moved useInjectElementsIntoGrid = ( to it.
Added comment above
// Injects the size warning and Load buttons into the corners of EUI Data Grid