system logs ui changes #571

vishalmishraa · 2024-10-04T21:21:04Z

Linked Issue(s)

issue #555

Acceptance Criteria fulfillment

Task 1
Task 2
Task 3

Proposed changes (including videos or screenshots)

Further comments

jayantbh · 2024-10-04T21:53:15Z

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?

jayantbh · 2024-10-04T21:54:16Z

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.

vishalmishraa · 2024-10-04T22:25:42Z

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

jayantbh · 2024-10-05T09:03:17Z

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.

jayantbh · 2024-10-05T13:25:05Z

This PR could use some tests. Especially with various kinds of logs that might come through.

jayantbh · 2024-10-05T13:25:36Z

And of course, to test the performance of how many lines you're able to process via your functions as a benchmark.

jayantbh · 2024-10-05T13:50:13Z

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.

vishalmishraa · 2024-10-05T15:04:02Z

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.

jayantbh · 2024-10-06T07:54:27Z

No no. I'm saying you might not need to change your approach.

vishalmishraa · 2024-10-06T10:35:31Z

so , do i need to do any improvements or it is good to go ?

jayantbh · 2024-10-06T10:54:59Z

This PR could use some tests. Especially with various kinds of logs that might come through.

This.

vishalmishraa · 2024-10-06T12:27:00Z

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.

vishalmishraa · 2024-10-06T12:29:37Z

@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

jayantbh · 2024-10-06T15:37:46Z

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.

vishalmishraa · 2024-10-07T09:14:50Z

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.

jayantbh · 2024-10-07T21:24:48Z

I suspected it could go in this way.
You can go with a simpler approach altogether as well.

vishalmishraa · 2024-10-07T21:29:56Z

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 ?

…ata synchronization logs

jayantbh · 2024-10-08T10:46:00Z

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.

vishalmishraa · 2024-10-08T11:44:03Z

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, and LOG_WARNING respectively. 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.

jayantbh · 2024-10-10T08:29:31Z

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

vishalmishraa · 2024-10-11T20:44:25Z

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.

vishalmishraa · 2024-10-11T20:55:33Z

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 .

vishalmishraa · 2024-10-14T20:09:52Z

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.

jayantbh · 2024-10-15T07:55:29Z

@vishalmishraa could you share screenshots of the error boundary in effect?

vishalmishraa · 2024-10-15T08:42:30Z

@vishalmishraa could you share screenshots of the error boundary in effect?

Sure :

Error.Test.mov

jayantbh · 2024-10-15T08:45:16Z

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.

vishalmishraa · 2024-10-15T08:48:10Z

i think linter is failed because of workflow

jayantbh · 2024-10-15T09:02:23Z

Okay, I guess ignore that for now. We'll do the reviews as soon as we're able. :)

e-for-eshaan · 2024-10-15T14:44:31Z

web-server/src/utils/logFormatter.ts

+export const parseLogLine = (rawLogLine: string): ParsedLog | null => {
+  const generalLogMatch = rawLogLine.match(generalLogRegex);
+  if (generalLogMatch) {
+    const [, timestamp, , logLevel, message] = generalLogMatch;


const [_pickAname, timestamp, _pickAnotherName, logLevel, message]

e-for-eshaan · 2024-10-15T14:46:10Z

web-server/src/utils/logFormatter.ts

+    const [method, path] = request.split(' ');
+    return {
+      timestamp,
+      logLevel: 'INFO', // Assuming all HTTP logs are INFO level


all these string constants should be wrapped in an enum in our src/types/resources.ts

enum YourEnumName { INFO = 'INFO', DEBUG = 'DEBUG' ...other_possible_values... }

done , created a enum LogLevel.

e-for-eshaan · 2024-10-15T14:46:50Z

web-server/src/utils/logFormatter.ts

+
+  const postgresLogMatch = rawLogLine.match(postgresLogRegex);
+  if (postgresLogMatch) {
+    const [, timestamp, , logLevel, message] = postgresLogMatch;


same:

const [_pickAname, timestamp, _pickAnotherName, logLevel, message]

e-for-eshaan · 2024-10-15T14:47:34Z

web-server/src/utils/logFormatter.ts

+
+  const dataSyncLogMatch = rawLogLine.match(dataSyncLogRegex);
+  if (dataSyncLogMatch) {
+    const [, logLevel, action, service, message] = dataSyncLogMatch;


e-for-eshaan · 2024-10-15T14:48:55Z

web-server/src/constants/log-formatter.ts

+export const validLogLevels = new Set([
+  'DEBUG',
+  'INFO',
+  'NOTICE',
+  'WARNING',
+  'ERROR',
+  'LOG',
+  'FATAL',
+  'PANIC',
+  'STATEMENT',
+  'DETAIL'
+]);


this should be an enum in our resources file

I added these as enums and also removed the validLogLevels logic from the parser. Previously, I was using two approaches to parse PostgreSQL logs, but during testing, I noticed that my first approach, PostgresMultilineLogMatch, can parse both types of PostgreSQL logs. Therefore, there's no need to write separate logic for each type ,refer to the official postgres documentation . and this article also for log levels .

e-for-eshaan · 2024-10-16T08:39:28Z

web-server/src/utils/logFormatter.ts

+export const parseLogLine = (rawLogLine: string): ParsedLog | null => {
+  const generalLogMatch = rawLogLine.match(generalLogRegex);
+  if (generalLogMatch) {
+    const [_fullLog, timestamp, _unused, logLevel, message] = generalLogMatch;


_unused
this looks off, name it correctly to what this variable stands for, and the underscore is enough to know that this variable is unused

done. ,changing it with _pid

e-for-eshaan · 2024-10-16T08:40:12Z

web-server/src/utils/logFormatter.ts

+      _unused,
+      _unused2,


same with these :)

changed the second one to _username , and first one with _ (underscore) refer to this

e-for-eshaan

LGTM, good stuff @vishalmishraa 🎉

shivam-bit · 2024-10-18T05:34:15Z

web-server/src/constants/log-formatter.ts

+export interface ParsedLog {
+  timestamp: string;
+  logLevel: string;
+  message: string;
+  role?: string;
+  ip?: string;
+}


move this interface declaration to resources.ts. we use that for all our type declaration

shivam-bit · 2024-10-18T05:40:05Z

web-server/src/components/Service/SystemLog/PlainLog.tsx

+    <Line
+      key={index}
+      marginBottom={'8px'}
+      fontSize={'14px'}
+      fontFamily={'monospace'}
+    >


shivam-bit · 2024-10-18T05:54:23Z

web-server/src/hooks/useSystemLogs.tsx

+  const logs = useMemo(() => {
+    return services[serviceName]?.logs || [];
+  }, [serviceName, services]);


const logs = useMemo(() => services[serviceName]?.logs || [], [serviceName, services]);

shivam-bit · 2024-10-18T06:06:48Z

web-server/src/components/Service/SystemLog/FormattedLog.tsx

+    <Line
+      key={index}
+      marginBottom={'8px'}
+      fontSize={'14px'}
+      fontFamily={'monospace'}


same as other requested changes

shivam-bit · 2024-10-18T06:08:02Z

web-server/src/components/Service/SystemLog/FormattedLog.tsx

+      fontSize={'14px'}
+      fontFamily={'monospace'}
+    >
+      <Line component="span" color="info.main">


i think you can do this like <Line color="info">

shivam-bit · 2024-10-18T07:29:46Z

web-server/src/components/Service/SystemLog/FormattedLog.tsx

+
+  const { timestamp, ip, logLevel, message } = log;
+  return (
+    <Line medium marginBottom={'8px'} fontFamily={'monospace'}>


Try this<Line mono marginBottom={1}>

shivam-bit · 2024-10-18T07:30:42Z

web-server/src/components/Service/SystemLog/PlainLog.tsx

+
+export const PlainLog = ({ log }: { log: string; index: number }) => {
+  return (
+    <Line medium marginBottom={'8px'} fontFamily={'monospace'}>


Try this <Line mono marginBottom={1}>, it should ideally give you the same effect.

shivam-bit · 2024-10-18T12:49:00Z

@vishalmishraa LGTM!🚀🚀🚀

jayantbh · 2024-10-19T12:19:35Z

I think this doesn't need another round of reviews. We can go ahead and merge this. Any remaining changes will probably be minor and can be taken up in subsequent PRs.

jayantbh · 2024-10-19T12:20:20Z

Congrats on getting this merged!
Good work my dude!

system logs ui changes

f2c1382

refact required codes

d1d83cd

removed metadata & fixed linting

e531aa7

vishalmishraa marked this pull request as draft October 7, 2024 09:17

Merge branch 'middlewarehq:main' into system-logs-ui

b28a45b

Refactor logFormatter.ts to improve log parsing and add support for d…

98fd700

…ata synchronization logs

vishalmishraa added 2 commits October 8, 2024 17:05

improved redis parser

8c7e282

Add tests for logFormatter

591602d

vishalmishraa marked this pull request as ready for review October 8, 2024 11:39

Merge branch 'middlewarehq:main' into system-logs-ui

df905e9

Implementation of ErrorBoundary

26dabfa

jayantbh mentioned this pull request Oct 15, 2024

enhancements for system logs UI #590

Closed

e-for-eshaan requested changes Oct 15, 2024

View reviewed changes

refactore : Postgres log parsing logic and enum declaration.

c2c65ec

e-for-eshaan requested changes Oct 16, 2024

View reviewed changes

fix variables names in Parser

55f404e

vishalmishraa requested a review from e-for-eshaan October 17, 2024 09:50

e-for-eshaan previously approved these changes Oct 18, 2024

View reviewed changes

shivam-bit suggested changes Oct 18, 2024

View reviewed changes

refactore codes

b26aa4e

vishalmishraa dismissed e-for-eshaan’s stale review via b26aa4e October 18, 2024 06:41

vishalmishraa requested a review from shivam-bit October 18, 2024 06:42

shivam-bit suggested changes Oct 18, 2024

View reviewed changes

Updated margin and font styles for formatted and plain logs

9ec9da3

vishalmishraa requested a review from shivam-bit October 18, 2024 10:22

shivam-bit approved these changes Oct 18, 2024

View reviewed changes

shivam-bit requested a review from e-for-eshaan October 18, 2024 12:49

jayantbh merged commit b524c08 into middlewarehq:main Oct 19, 2024
3 checks passed

vishalmishraa deleted the system-logs-ui branch October 19, 2024 13:45

system logs ui changes #571

system logs ui changes #571

Conversation

vishalmishraa commented Oct 4, 2024

Linked Issue(s)

Acceptance Criteria fulfillment

Proposed changes (including videos or screenshots)

Further comments

jayantbh commented Oct 4, 2024

jayantbh commented Oct 4, 2024

vishalmishraa commented Oct 4, 2024 • edited Loading

jayantbh commented Oct 5, 2024

jayantbh commented Oct 5, 2024

jayantbh commented Oct 5, 2024

jayantbh commented Oct 5, 2024

vishalmishraa commented Oct 5, 2024

jayantbh commented Oct 6, 2024

vishalmishraa commented Oct 6, 2024

jayantbh commented Oct 6, 2024

vishalmishraa commented Oct 6, 2024 • edited Loading

vishalmishraa commented Oct 6, 2024 • edited Loading

UPDATE : got some different kind of logs from my own dbs , gonna use them in testing :

FOR REDIS :

FOR POSTGRES:

jayantbh commented Oct 6, 2024

vishalmishraa commented Oct 7, 2024 • edited Loading

jayantbh commented Oct 7, 2024

vishalmishraa commented Oct 7, 2024 • edited Loading

jayantbh commented Oct 8, 2024

vishalmishraa commented Oct 8, 2024 • edited Loading

Reference for Regexes

Redis Log Regex

HTTP Gunicorn Log Regex

jayantbh commented Oct 10, 2024 • edited Loading

vishalmishraa commented Oct 11, 2024

vishalmishraa commented Oct 11, 2024

vishalmishraa commented Oct 14, 2024

jayantbh commented Oct 15, 2024

vishalmishraa commented Oct 15, 2024

Sure :

jayantbh commented Oct 15, 2024

vishalmishraa commented Oct 15, 2024 • edited Loading

jayantbh commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-for-eshaan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivam-bit Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivam-bit commented Oct 18, 2024

jayantbh commented Oct 19, 2024

jayantbh commented Oct 19, 2024

vishalmishraa commented Oct 4, 2024 •

edited

Loading

vishalmishraa commented Oct 6, 2024 •

edited

Loading

vishalmishraa commented Oct 6, 2024 •

edited

Loading

vishalmishraa commented Oct 7, 2024 •

edited

Loading

vishalmishraa commented Oct 7, 2024 •

edited

Loading

vishalmishraa commented Oct 8, 2024 •

edited

Loading

jayantbh commented Oct 10, 2024 •

edited

Loading

vishalmishraa commented Oct 15, 2024 •

edited

Loading

shivam-bit Oct 18, 2024 •

edited

Loading