system logs ui changes
Linked Issue(s)
issue #555
Acceptance Criteria fulfillment
- [ ] Task 1
- [ ] Task 2
- [ ] Task 3
Proposed changes (including videos or screenshots)
Further comments
Thanks for your contribution, @vishalmishraa! Based on the screenshots it's certainly a decent improvement.
I would love to accept this PR, but it needs a few improvements to avoid performance issues.
Given that there could be thousands of lines being rendered, we do need to keep an eye out for excessive calls to JSON.stringify, Array.includes, and various other places.
Additionally, I think the metadata doesn't really need to be printed. What do you think?
Linter failed. We need to sort out the linting issues but for now you can check our contributing guide to see how to fix that. Let us know if it doesn't work out for you.
Thanks for your contribution, @vishalmishraa! Based on the screenshots it's certainly a decent improvement.
I would love to accept this PR, but it needs a few improvements to avoid performance issues.
Given that there could be thousands of lines being rendered, we do need to keep an eye out for excessive calls to JSON.stringify, Array.includes, and various other places.
Additionally, I think the metadata doesn't really need to be printed. What do you think?
hi , @jayantbh ,
I’ll remove the metadata entirely, so there’s no need to use JSON.stringify. for array.includes, I’ll use Set() instead, as it takes O(1) time complexity, which is a better approach. . am I taking the correct approach? and yup, I need to fix the linting issues
Directionally I think yes. Your last commit looks better too.
I haven't done a proper review but I'll ask the team to check it out.
This PR could use some tests. Especially with various kinds of logs that might come through.
And of course, to test the performance of how many lines you're able to process via your functions as a benchmark.
I ran a quick test myself based on the number of lines, and I think we'll hit other performance limits before we hit perf issues with the regexes and matches.
I ran a quick test myself based on the number of lines, and I think we'll hit other performance limits before we hit perf issues with the regexes and matches.
okay , i think i need to change my approach ,Could you suggest an alternatives or any way to do that ? I will work on implementing it accordingly.
No no. I'm saying you might not need to change your approach.
so , do i need to do any improvements or it is good to go ?
This PR could use some tests. Especially with various kinds of logs that might come through.
This.
Okay, I’m working on it. I’ll be writing tests in ./web-server/src/utils/test, where I’ll create a logFormatter.test.ts file and write the tests.
@jayantbh I need help regarding generating different types of system logs. Should I generate new logs for testing? If yes, how can I generate other different logs? Or is it better to use the logs that are currently being generated and write test for them only?
UPDATE : got some different kind of logs from my own dbs , gonna use them in testing :
FOR REDIS :
FOR POSTGRES:
AND i think for gunicorn the default system generated logs are enogh
I think handle for the variety of logs that are generated today. But ensure that when and if something unexpected comes in the logs (such as, missing HTTP method in HTTP logs, dates formatted in a different way, etc.) Then it doesn't break the UI.
i think the function I wrote is not feasible for parsing all types of logs. There are several different types of logs, such as system-generated logs (some print statements in backend code), HTTP server error logs, and others.
i did some research on better logging solutions and found a CNCF-based library called Fluentd, which offers unified logging, better parsing, and improved data collection and observation. It's also lightweight, requiring only 40 to 50 MB of space. I think it could be a good fit for this project.
more details:
since , i think current approach may not be the right fit right now, I’m focusing on other way to solve it.
I suspected it could go in this way. You can go with a simpler approach altogether as well.
I suspected it could go in this way. You can go with a simpler approach altogether as well.
Then , there will be some logs, which can't be parsed, and they will be displayed as it is without colors. I'll try to add regex for logs that are frequently generated, should i proceed with this ?
I suspected it could go in this way. You can go with a simpler approach altogether as well.
Then , there will be some logs, which can't be parsed, and they will be displayed as it is without colors. I'll try to add regex for logs that are frequently generated, should i proceed with this ?
Yea, I think you could try that.
Something is better than nothing after all. But make sure you have tests.
I suspected it could go in this way. You can go with a simpler approach altogether as well.
Then , there will be some logs, which can't be parsed, and they will be displayed as it is without colors. I'll try to add regex for logs that are frequently generated, should i proceed with this ?
Yea, I think you could try that.
Something is better than nothing after all. But make sure you have tests.
Thanks, @jayantbh. I've updated the functions and added the tests. Please review them.
Reference for Regexes
Redis Log Regex
-
Log Level: Represents log levels (., -, *, #) which correspond to
LOG_DEBUG,LOG_INFO,LOG_NOTICE, andLOG_WARNINGrespectively. For more information, refer to the Redis source code. -
Role: The role is represented by (X, C, S, or M). For details, see in the Redis source code.
HTTP Gunicorn Log Regex
For details on the HTTP Gunicorn log regex, refer to the official Gunicorn documentation.
Looks better. :)
Could you use Error Boundaries to show the logs in completely plain-text (the view before your changes that is) in case something breaks in the off chance?
Basically the idea is, that if something does break, it should at least show the plain version of the logs. Of course they should be shown in a tiny text at the top or bottom, or somewhere that they ran into an error and they should create an issue, which should link to: https://github.com/middlewarehq/middleware/issues/new/choose
I attempted to use the default ErrorBoundary, but when I do so,i'm not able to use coustom fallback component which display plain logs with message for user to report because a higher-level ErrorBoundary is catching the error before it reaches the custom fallback that I am trying to render in plain text.
Proposed Solution:
- I will create an
errorvariable to store any caught errors. - The formatted logs will be rendered only when there is no error.
- I'll wrap the formatted log rendering code in a
try-catchblock to capture any errors. If an error occurs, I'll set the error in the state.
Here's an example of the implementation:
return (
<FlexBox col>
{loading ? (
<FlexBox alignCenter gap2>
<CircularProgress size="20px" />
<Line>Loading...</Line>
</FlexBox>
) : (
(!error && services) ? (
logs.map((log, index) => {
try {
// Rest of the code
} catch (error) {
setError(error);
}
})
) : (
<SystemLogsErrorFallback error={error} serviceName={serviceName} />
)
)}
<FlexBox ref={containerRef} />
</FlexBox>
);
- If there is an error, the
SystemLogErrorFallback.tsxcomponent will be rendered. This component will display the logs in plain text with an appropriate error message.
OUTCOME :
Should I proceed with this approach, or is there a better way to handle this scenario?
Hmm, not sure I feel great about that approach. How did you try to add and test the error boundary that it caused a higher level boundary to catch it instead?
Hmm, not sure I feel great about that approach. How did you try to add and test the error boundary that it caused a higher level boundary to catch it instead?
i did this :
const SystemLogs = () => {
return (
<ErrorBoundary FallbackComponent={SystemLogErrorFallback}>
// code
</ErrorBoundary>
);
};
But it didn’t work, and I think it should have worked.
Also i think the approach I'm using now would be work good because the component which should be passed into the ErrorBoundary fallback should also receive serviceName and other props as it should render logs in plain text. So, in the future, if we make any improvements to the UI for SystemLogs like (implementing search feature ,etc..), we would need to apply the same modifications to the custom fallback component as well, since it's rendering system logs in plain text. Therefore, it would be better to handle this in the same position .
Hi @jayantbh ,
I’ve implemented the error boundary and apologize for the confusion earlier. You were right , I was testing it incorrectly, which is why it didn’t work. However, I’ve now implemented it properly. I’ve also created a useSystemLog hook for it and separated the component.Please check it now.
@vishalmishraa could you share screenshots of the error boundary in effect?
@vishalmishraa could you share screenshots of the error boundary in effect?
Sure :
https://github.com/user-attachments/assets/e05ff22a-ad98-474b-892f-105bd3b3eef6
Looks great! The linter appears to have failed. Do check what that's about.
Meanwhile, we'll give this a round of final reviews. But I think we're looking good visually/functionally.
i think linter is failed because of workflow
Okay, I guess ignore that for now. We'll do the reviews as soon as we're able. :)