-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add support for viewing search results in context for text logs (clp-text). #489
Conversation
@@ -22,6 +22,14 @@ const plugins = [ | |||
]; | |||
|
|||
const config = { | |||
devServer: { | |||
proxy: [ |
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.
Proxy added to avoid CORS issues during development.
@@ -77,6 +78,12 @@ const SearchResultsTable = ({ | |||
} | |||
}; | |||
|
|||
const handleSearchResultClick = (ev) => { | |||
const {orig_file_id: origFileId, log_event_ix: logEventIx} = ev.currentTarget.dataset; | |||
window.open(`${Meteor.settings.public.LogViewerWebuiClientUrl |
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.
In the future, we can open the Log Viewer in a split pane so users can see both the CLP search results and the log context at the same time. For the MVP, we can open the Log Viewer WebUI client in a new tab. Feel free to object.
hey @haiqi96 , could you help triage this review? |
The change looks reasonable to me, didn't notice any obvious issue |
@@ -786,6 +786,7 @@ def start_webui(instance_id: str, clp_config: CLPConfig, mounts: CLPDockerMounts | |||
}, | |||
"public": { | |||
"ClpStorageEngine": clp_config.package.storage_engine, | |||
"LogViewerWebuiClientUrl": f"http://{clp_config.log_viewer_webui.host}:{clp_config.log_viewer_webui.port}", |
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.
maybe just LogViewerWebuiUrl
?
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.
Sg
/** | ||
* Renders the main application. | ||
* | ||
* @return {JSX.Element} | ||
*/ | ||
const App = () => { | ||
return ( | ||
<h1>Hello world!</h1> | ||
<CssVarsProvider modeStorageKey={"uiTheme"}> |
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.
create a const for string "uiTheme"
components/webui/settings.json
Outdated
@@ -13,6 +13,7 @@ | |||
"AggregationResultsCollectionName": "aggregation-results", | |||
"ClpStorageEngine": "clp", | |||
"CompressionJobsCollectionName": "compression-jobs", | |||
"LogViewerWebuiClientUrl": "http://localhost:8080", |
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.
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.
How about QueryStatus.jsx
?
* | ||
* @return {React.ReactElement} | ||
*/ | ||
const Query = () => { |
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.
Can we name this the same as the file?
components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/index.jsx
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
…ever applicable.
…_LOAD_STATE` as indices.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
…yperlinked log message text approach.
…o React components.
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.
A few more suggestions.
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
Outdated
Show resolved
Hide resolved
* | ||
* @enum {QueryLoadState} | ||
*/ | ||
const QUERY_LOAD_STATE = Object.freeze({ |
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.
QUERY_LOADING_STATES
?
/** | ||
* Descriptions for query states. | ||
*/ | ||
const QUERY_STATE_DESCRIPTIONS = Object.freeze({ |
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.
QUERY_LOADING_STATE_DESCRIPTIONS
?
/** | ||
* @typedef {number} QueryLoadState | ||
*/ | ||
let enumQueryLoadState; |
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.
enumQueryLoadingState
?
components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/index.jsx
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
…ggestions from code review Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
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.
For the PR title, how about:
Add support for viewing search results in context for text logs (clp-text).
Description
These hyperlinks redirect users to the Log Viewer WebUI, which will:
This integration streamlines the workflow, making it easier for users to navigate from search results to detailed log analysis.
Validation performed