chore: SDK Truncation Logic #1347
Conversation
JoshuaMoelans
changed the title
- reason: span tags are a legacy concept so we're not touching it ~dixit ingest
|
To remove the Since the only have index-based envelope item access happens inside our unit tests, there is no real performance impact. This allows for attachment counts limited only by available memory. |
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| // Initialize item | ||
| item->headers = sentry_value_new_object(); | ||
| item->event = sentry_value_new_null(); | ||
| item->payload = NULL; |
There was a problem hiding this comment.
In envelope_add_item(), when item->headers = sentry_value_new_object() fails (returns NULL), the function continues and returns an item with a NULL headers field. The sentry_value_decref() call on NULL headers during cleanup could cause issues. Consider checking if sentry_value_new_object() succeeded before proceeding.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry_envelope.c#L56-L60
Potential issue: In `envelope_add_item()`, when `item->headers =
sentry_value_new_object()` fails (returns NULL), the function continues and returns an
item with a NULL headers field. The `sentry_value_decref()` call on NULL headers during
cleanup could cause issues. Consider checking if `sentry_value_new_object()` succeeded
before proceeding.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2806251
There was a problem hiding this comment.
if sentry_value_new_object fails, it returns sentry_value_new_null which we can still decref.
| if (envelope->is_raw) { | ||
| return NULL; | ||
| } | ||
| if (envelope->contents.items.item_count >= SENTRY_MAX_ENVELOPE_ITEMS) { | ||
| return NULL; | ||
| } | ||
|
|
||
| // TODO: Envelopes may have at most one event item or one transaction item, | ||
| // and not one of both. Some checking should be done here or in | ||
| // `sentry__envelope_add_[transaction|event]` to ensure this can't happen. | ||
|
|
||
| sentry_envelope_item_t *rv | ||
| = &envelope->contents.items | ||
| .items[envelope->contents.items.item_count++]; | ||
| rv->headers = sentry_value_new_object(); | ||
| rv->event = sentry_value_new_null(); | ||
| rv->payload = NULL; | ||
| rv->payload_len = 0; | ||
| return rv; | ||
| // Allocate new item | ||
| sentry_envelope_item_t *item = SENTRY_MAKE(sentry_envelope_item_t); | ||
| if (!item) { | ||
| return NULL; | ||
| } | ||
|
|
||
| // Initialize item | ||
| item->headers = sentry_value_new_object(); | ||
| item->event = sentry_value_new_null(); | ||
| item->payload = NULL; | ||
| item->payload_len = 0; | ||
| item->next = NULL; | ||
|
|
||
| // Append to linked list | ||
| if (envelope->contents.items.last_item) { | ||
| envelope->contents.items.last_item->next = item; | ||
| } else { | ||
| envelope->contents.items.first_item = item; | ||
| } | ||
| envelope->contents.items.last_item = item; | ||
| envelope->contents.items.item_count++; | ||
|
|
||
| return item; | ||
| } |
There was a problem hiding this comment.
The change from a fixed-size array to dynamic linked list removes the hard limit on envelope items. While the SENTRY_MAX_ENVELOPE_SESSIONS limit is used in sentry_database.c for sessions, there is no longer a check in envelope_add_item() to prevent unlimited growth of envelopes in general. This could potentially lead to memory exhaustion if envelopes are created with many items. Consider adding a check or documentation about expected envelope item limits.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry_envelope.c#L43-L74
Potential issue: The change from a fixed-size array to dynamic linked list removes the
hard limit on envelope items. While the `SENTRY_MAX_ENVELOPE_SESSIONS` limit is used in
`sentry_database.c` for sessions, there is no longer a check in `envelope_add_item()` to
prevent unlimited growth of envelopes in general. This could potentially lead to memory
exhaustion if envelopes are created with many items. Consider adding a check or
documentation about expected envelope item limits.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2806251
There was a problem hiding this comment.
that's the point of this PR :)
Fixes #1342
Truncation occurrences
MAX_ENVELOPE_ITEMSSee definition https://github.com/getsentry/sentry-native/blob/master/src/sentry_envelope.h#L13
This is only used in
envelope_add_item(here) which can only trigger for attachments (since the other APIs callingadd_itemall capture the envelope after adding the first item), so if a user sets more than 9 attachments we would lose the 10th onward.The Envelopes docs mention that we accept as most 100 sessions per envelope, which is presumably where this
MAX_ENVELOPE_ITEMSoriginated from. The question now is whether this also applies to other envelope items (like attachments/events/...)?-> we should only have the limit for sessions, not attachments (or other envelope items). we should check whether we can actually construct envelopes with multiple sessions, and if so, still limit this to 100 (not 10). Remove 'truncating'(dropping) envelope items for any other type of telemetry. ( ✅ updated session test )
Out-of-Scope
Stack Traces
It was decided that Stack Trace Truncation was out of scope for this first round of SDK truncation logic updates (TODO: link new Linear ticket for this topic)
🔍
sentry_unwind_stack(..., max_frames)Code here. Need to investigate if this is something we should be truncating still, or if Relay can handle this.
🔍 inproc
MAX_FRAMESCode here and used for unwinding the stack here
Keep non-truncated
✅
sentry_set_tagCorrectly gets truncated to 200 characters in Relay (count including ...)
Keep truncation
Span Tags
We keep truncation because
The following was observed with this snippet:
Code Snippet
```c sentry_transaction_context_t *tx_ctx = sentry_transaction_context_new("my_tx", "op"); sentry_transaction_t *tx = sentry_transaction_start(tx_ctx, sentry_value_new_null()); sentry_span_t *span = sentry_transaction_start_child(tx, "span_op", "span_desc"); char *tag = "supermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagname"; char *tag_val = "supermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagnamesupermegalongtagname"; sentry_set_tag(tag, tag_val); sentry_span_set_tag(span, tag, tag_val); sentry_span_finish(span); sentry_transaction_finish(tx); sentry_value_t event = sentry_value_new_message_event( SENTRY_LEVEL_INFO, "my-logger", "Hello World!"); ```❓
sentry_span_set_tag(from a prior discussion, TBD if this is still true)
(UPDATE 2025-11-13) -> we see correct truncation on transactions, but not on spans yet
code here and mention of truncation for
sentry_span_set_taghereremoved in a9712b2 but seems like this is not truncated during ingestion... docs mention that tags must be less than 200 characters
Product doesn't seem to like these large values for searchability
