-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: add safeguard for large log body entries #8560
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
Conversation
Aditya Singh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
Caution
Changes requested β
Reviewed everything up to b80f342 in 1 minute and 54 seconds. Click for details.
- Reviewed
458
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/components/LogDetail/index.tsx:178
- Draft comment:
Avoid inline styles for the close icon; consider moving the margin styling (e.g. marginTop: Spacing.MARGIN_1) into a CSS class. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/container/LogDetailedView/TableView/TableViewActions.tsx:79
- Draft comment:
Avoid using inline styles (e.g. the style object with display, alignItems, and gap) for the loading spinner container. Consider moving these styles to an external stylesheet or CSS class for consistency with design guidelines. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:41
- Draft comment:
Good safeguard implemented: the hook checks the byte size of the JSON and skips processing if it exceeds MAX_BODY_BYTES. This should effectively prevent huge log bodies from breaking the UI. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_IMlQ2PKKljgGOYIm
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts
Outdated
Show resolved
Hide resolved
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.
Important
Looks good to me! π
Reviewed dea8145 in 2 minutes and 14 seconds. Click for details.
- Reviewed
164
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/container/LogDetailedView/util.test.ts:194
- Draft comment:
Consider adding a test case for the large log body safeguard. The current tests for getSanitizedLogBody do not verify behavior (e.g., truncation or fallback) when the input exceeds a set threshold. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests testing a feature (large log body safeguards) that isn't evident in the code. Without seeing the implementation of getSanitizedLogBody, I can't verify if such safeguards exist. The comment is speculative about what should be tested rather than pointing out a clear issue with existing code. I might be missing evidence of large log body handling in the implementation file. The feature could exist but just be untested. Even if the feature exists, without seeing clear evidence in the implementation, keeping a speculative comment about testing would violate the rule about making speculative comments. Delete the comment as it makes assumptions about functionality without clear evidence and is speculative in nature.
2. frontend/src/container/LogDetailedView/util.test.ts:1
- Draft comment:
The PR title and description mention fixes for large log body entries (and URL parameter issues), but the diff only includes tests for log sanitization. Please ensure that all intended issues are addressed in this PR. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that all intended issues are addressed, which violates the rule against asking the author to confirm their intention or ensure behavior. It also indirectly asks for an update to the PR description, which is not allowed.
3. frontend/src/container/LogDetailedView/util.test.ts:49
- Draft comment:
The 'escaped characters' test for recursiveParseJSON uses a JSON string that appears to have incorrect escaping. Verify the intended format and expected output of this test. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_x0xeFSXLcnxroWpH
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Important
Looks good to me! π
Reviewed fdc4b53 in 1 minute and 16 seconds. Click for details.
- Reviewed
27
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/container/LogDetailedView/utils.tsx:15
- Draft comment:
Caching the Convert instance improves performance, but ensure that this instance is stateless. If Convert maintains any internal state per conversion, consider instantiating it per call. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/container/LogDetailedView/utils.tsx:353
- Draft comment:
The PR title mentions adding a safeguard for large log body entries, but no logic to check or truncate large log entries is present here. Ensure that the safeguard is implemented as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that a safeguard is implemented, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for a specific test to be written.
Workflow ID: wflow_Qpo8TlbaMwt3HBOz
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Important
Looks good to me! π
Reviewed cf11fcd in 1 minute and 10 seconds. Click for details.
- Reviewed
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:36
- Draft comment:
Good change: measuring byte size directly from the string (since 'value' is a string) avoids extra characters from JSON.stringify. Ensure this behavior is validated with edge cases. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
2. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:38
- Draft comment:
If the log body exceeds MAX_BODY_BYTES, the hook returns without updating state. Consider setting an error state (or a placeholder) so the UI can notify the user that the log is too large, as suggested by the PR description. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:1
- Draft comment:
The PR description also mentions resolving an API call parameter passing issue, but this diff only addresses the large log safeguard. Please verify that all intended issues are covered in the PR. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_T29yQWEg3NCPSaRu
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Important
Looks good to me! π
Reviewed 711bb41 in 2 minutes and 18 seconds. Click for details.
- Reviewed
37
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/container/LogDetailedView/TableView/TableViewActions.tsx:63
- Draft comment:
Good: Removed the unnecessary console.log debugging statement. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/container/LogDetailedView/TableView/TableViewActions.tsx:1
- Draft comment:
Note: The PR description mentions an API call parameter issue (e.g., missing URL params on /traces), but the diff only addresses safeguarding large log body entries. Please confirm that the intended issue is being resolved. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:36
- Draft comment:
The safeguard to avoid processing huge JSON bodies by checking the byte size is in place. However, if the log body exceeds 100KB, the processing is simply skipped without updating the state to reflect an error. Consider setting an explicit error state (e.g. 'Log body too large') so that the UI can display a placeholder (such as '{}') as suggested in the PR description. - Reason this comment was not posted:
Comment was on unchanged code.
4. frontend/src/container/LogDetailedView/utils.tsx:14
- Draft comment:
The comment about caching the Convert instance was removed. Ensure that the Convert instance remains cached (or appropriately reused) if performance is a concern, even if an explicit caching comment is no longer present. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. frontend/src/container/LogDetailedView/TableView/useAsyncJSONProcessing.ts:36
- Draft comment:
Typo: Consider changing 'json' to 'JSON' for consistency with standard naming conventions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is an extremely minor stylistic suggestion about capitalization in a comment. It doesn't affect functionality, readability, or maintainability in any meaningful way. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category. Additionally, there's no strong evidence that this change is actually required by any style guide. Perhaps maintaining consistent capitalization across the codebase could be important for professionalism and attention to detail? While consistency is good, this level of nitpicking about capitalization in comments doesn't provide enough value to justify a PR comment and the overhead of making the change. Delete this comment as it's too minor and doesn't provide meaningful value to the codebase.
Workflow ID: wflow_PcHOZmQS18D2MY6N
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
π Summary
Log entries have size more than a certain threshold breaking UI. Added safeguards to prevent same.
β Changes
π Related Issues
Closes #
https://github.com/SigNoz/engineering-pod/issues/2656
πΈ Screenshots / Screen Recording (if applicable / mandatory for UI related changes)
Before:
Large Log breaking UI
Screen.Recording.2025-07-18.at.4.21.42.PM.mov
After:
On error processing data displaying : {} (similar to datadog, can change later if req)
Screen.Recording.2025-07-18.at.4.24.36.PM.mov
π Checklist
π Notes for Reviewers
Important
Add safeguard for large log entries and unify log body sanitization across components.
useAsyncJSONProcessing
to skip processing JSON bodies larger than 100 KB.getSanitizedLogBody
to sanitize log bodies across components, ensuring consistent handling.LogDetail
,ListLogView
,RawLogView
, andTableView
to usegetSanitizedLogBody
for log body rendering.TableViewActions
to handle large log bodies with async processing.getSanitizedLogBody
function inutils.tsx
for consistent log body sanitization.getSanitizedLogBody
inutil.test.ts
to cover various input scenarios.This description was created by
for 711bb41. You can customize this summary. It will automatically update as commits are pushed.