middleware icon indicating copy to clipboard operation
middleware copied to clipboard

system logs ui changes

Open vishalmishraa opened this issue 1 year ago • 24 comments

Linked Issue(s)

issue #555

Acceptance Criteria fulfillment

  • [ ] Task 1
  • [ ] Task 2
  • [ ] Task 3

Proposed changes (including videos or screenshots)

Screenshot 2024-10-05 at 1 53 58 AM Screenshot 2024-10-05 at 1 50 18 AM Screenshot 2024-10-05 at 1 50 03 AM Screenshot 2024-10-05 at 1 50 11 AM

Further comments

vishalmishraa avatar Oct 04 '24 21:10 vishalmishraa

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 avatar Oct 04 '24 21:10 jayantbh

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.

jayantbh avatar Oct 04 '24 21:10 jayantbh

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

vishalmishraa avatar Oct 04 '24 22:10 vishalmishraa

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 avatar Oct 05 '24 09:10 jayantbh

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

jayantbh avatar Oct 05 '24 13:10 jayantbh

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

jayantbh avatar Oct 05 '24 13:10 jayantbh

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.

jayantbh avatar Oct 05 '24 13:10 jayantbh

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.

vishalmishraa avatar Oct 05 '24 15:10 vishalmishraa

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

jayantbh avatar Oct 06 '24 07:10 jayantbh

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

vishalmishraa avatar Oct 06 '24 10:10 vishalmishraa

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

This.

jayantbh avatar Oct 06 '24 10:10 jayantbh

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 avatar Oct 06 '24 12:10 vishalmishraa

@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 :

Screenshot 2024-10-06 at 9 00 56 PM

FOR POSTGRES:

Screenshot 2024-10-06 at 9 01 26 PM

AND i think for gunicorn the default system generated logs are enogh

vishalmishraa avatar Oct 06 '24 12:10 vishalmishraa

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.

jayantbh avatar Oct 06 '24 15:10 jayantbh

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.

vishalmishraa avatar Oct 07 '24 09:10 vishalmishraa

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

jayantbh avatar Oct 07 '24 21:10 jayantbh

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 ?

vishalmishraa avatar Oct 07 '24 21:10 vishalmishraa

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.

jayantbh avatar Oct 08 '24 10:10 jayantbh

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.

vishalmishraa avatar Oct 08 '24 11:10 vishalmishraa

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

jayantbh avatar Oct 10 '24 08:10 jayantbh

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 error variable 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-catch block 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.tsx component will be rendered. This component will display the logs in plain text with an appropriate error message.

OUTCOME :

Screenshot 2024-10-10 at 9 11 44 PM

Should I proceed with this approach, or is there a better way to handle this scenario?

vishalmishraa avatar Oct 10 '24 15:10 vishalmishraa

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?

jayantbh avatar Oct 11 '24 20:10 jayantbh

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 avatar Oct 11 '24 20:10 vishalmishraa

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 avatar Oct 11 '24 20:10 vishalmishraa

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 avatar Oct 14 '24 20:10 vishalmishraa

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

jayantbh avatar Oct 15 '24 07:10 jayantbh

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

Sure :

https://github.com/user-attachments/assets/e05ff22a-ad98-474b-892f-105bd3b3eef6

vishalmishraa avatar Oct 15 '24 08:10 vishalmishraa

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.

jayantbh avatar Oct 15 '24 08:10 jayantbh

i think linter is failed because of workflow

Screenshot 2024-10-15 at 2 15 56 PM

vishalmishraa avatar Oct 15 '24 08:10 vishalmishraa

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

jayantbh avatar Oct 15 '24 09:10 jayantbh