Skip to content

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

Merged
merged 7 commits into from
Jul 23, 2025
Merged

fix: add safeguard for large log body entries #8560

merged 7 commits into from
Jul 23, 2025

Conversation

aks07
Copy link
Contributor

@aks07 aks07 commented Jul 18, 2025

πŸ“„ Summary

Log entries have size more than a certain threshold breaking UI. Added safeguards to prevent same.


βœ… Changes

  • Feature: Brief description
  • Bug fix: Brief description

πŸ” 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

  • Dev Review
  • Test cases added (Unit/ Integration / E2E)
  • Manually tested the changes

πŸ‘€ Notes for Reviewers


Important

Add safeguard for large log entries and unify log body sanitization across components.

  • Behavior:
    • Add size check in useAsyncJSONProcessing to skip processing JSON bodies larger than 100 KB.
    • Use getSanitizedLogBody to sanitize log bodies across components, ensuring consistent handling.
  • Components:
    • Update LogDetail, ListLogView, RawLogView, and TableView to use getSanitizedLogBody for log body rendering.
    • Modify TableViewActions to handle large log bodies with async processing.
  • Utilities:
    • Add getSanitizedLogBody function in utils.tsx for consistent log body sanitization.
    • Add tests for getSanitizedLogBody in util.test.ts to cover various input scenarios.

This description was created by Ellipsis for 711bb41. You can customize this summary. It will automatically update as commits are pushed.

@aks07 aks07 requested review from YounixM and a team as code owners July 18, 2025 10:57
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


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.

@github-actions github-actions bot added the bug Something isn't working label Jul 18, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 7 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% <= threshold 50% None

Workflow ID: wflow_IMlQ2PKKljgGOYIm

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

YounixM
YounixM previously approved these changes Jul 23, 2025
@aks07 aks07 enabled auto-merge (squash) July 23, 2025 13:17
@aks07 aks07 merged commit b40fda0 into main Jul 23, 2025
12 of 13 checks passed
@aks07 aks07 deleted the fix/logs-crash branch July 23, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants