Skip to content

Considerably raise DEFAULT_MAX_VALUE_LENGTH #4632

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 12 commits into from
Jul 29, 2025

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Jul 28, 2025

AI prompts/messages are potentially huge.

  • raise DEFAULT_MAX_VALUE_LENGTH (responsible for string trimming) from 1024 to 100 000
  • adapt tests (and make them more generic, without hardcoded parts, where possible)

This does not seem to affect issue grouping, see e.g. https://sentry-sdks.sentry.io/issues/6774464781/events/latest/?project=4506716433416192&query=is%3Aunresolved&referrer=latest-event

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.74%. Comparing base (fd7dca4) to head (3ca41a0).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4632   +/-   ##
=======================================
  Coverage   80.74%   80.74%           
=======================================
  Files         156      156           
  Lines       16630    16630           
  Branches     2830     2830           
=======================================
  Hits        13428    13428           
  Misses       2312     2312           
  Partials      890      890           
Files with missing lines Coverage Δ
sentry_sdk/consts.py 94.73% <100.00%> (ø)

@sentrivana sentrivana marked this pull request as ready for review July 29, 2025 07:39
@sentrivana sentrivana requested a review from a team as a code owner July 29, 2025 07:39
@@ -3,7 +3,7 @@
from typing import TYPE_CHECKING

# up top to prevent circular import due to integration import
DEFAULT_MAX_VALUE_LENGTH = 1024
DEFAULT_MAX_VALUE_LENGTH = 100_000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use sys.maxsize here? Then, we definitely should never need to bump this limit again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, let's add a code comment to document how we picked the value we end up going with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked about this offline -- and added a comment

@sentrivana sentrivana enabled auto-merge (squash) July 29, 2025 11:34
1 / 0
except Exception:
capture_exception()

(event,) = events

assert len(json.dumps(event)) < 10000
assert len(json.dumps(event)) < DEFAULT_MAX_VALUE_LENGTH * 10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: String Truncation Test Assertion Too Weak

The assertion in test_databag_string_stripping is too weak, defeating the test's purpose. This test is designed to verify that large strings are truncated to keep serialized event sizes manageable. However, the current assertion len(json.dumps(event)) < DEFAULT_MAX_VALUE_LENGTH * 10 allows the serialized event to be as large as the input string (1,000,000 bytes, given DEFAULT_MAX_VALUE_LENGTH = 100_000). This fails to ensure that truncation is actually occurring and keeping the event size manageable. The assertion should be tightened to effectively test string stripping, similar to the original < 10000 bound.

Locations (1)
Fix in Cursor Fix in Web

@sentrivana sentrivana merged commit 4f9d326 into master Jul 29, 2025
138 checks passed
@sentrivana sentrivana deleted the ivana/bump-max-value-length-limit branch July 29, 2025 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants