Wire log_byte client outcome reporting alongside log_item#8185
Wire log_byte client outcome reporting alongside log_item#8185tsushanth wants to merge 1 commit into
Conversation
Adds kSentryDataCategoryLogByte (= "log_byte") as a new data category alongside the existing kSentryDataCategoryLogItem. The byte category tracks how many bytes of log envelope data were discarded, matching the client-reports spec at https://develop.sentry.dev/sdk/telemetry/client-reports/ Changes: - SentryDataCategory.h: add kSentryDataCategoryLogByte = 15; shift kSentryDataCategoryUnknown to 16 - SentryDataCategoryMapper.h/m: add kSentryDataCategoryNameLogByte constant ("log_byte") and wire it through sentryDataCategoryForString, nameForSentryDataCategory - SentryHttpTransport.m: add recordLostLogBytes:reason: helper that records item.data.length bytes under kSentryDataCategoryLogByte; call it alongside the existing recordLostEvent / recordLostSpans calls in envelopeItemDropped:withCategory:, envelopeItemDeleted:withCategory:, and recordLostEventFor: - SentryDataCategoryMapperTests.swift: extend integer, string, and name-to-category round-trip tests to cover kSentryDataCategoryLogByte
tsushanth
requested review from
NinjaLikesCheez,
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
NinjaLikesCheez
added
the
ready-to-merge
|
Thanks for the contribution! Allowing the CI to run - the code looks good to me, but I'd like a team member with more context on this type of change to weigh in and approve first. |
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
Almost LGTM, we have one topic to discuss
| - (void)recordLostLogBytes:(SentryEnvelopeItem *)envelopeItem reason:(SentryDiscardReason)reason | ||
| { | ||
| if ([SentryEnvelopeItemTypes.log isEqualToString:envelopeItem.type]) { | ||
| NSUInteger byteCount = envelopeItem.data.length; |
There was a problem hiding this comment.
m: We need to double-check if the data property is the actual size of the log item as we would count it server-side, or if it also contains boilerplate bytes like JSON encoding, attributes etc. For transaction we are actually decoding the data first.
There was a problem hiding this comment.
The byte size does not need to be exact. SDKs MAY use an approximation, such as the in-memory size of the log object or the size of its serialized representation. The goal is to provide a useful signal, not a precise measurement. - develop docs
@philprime I think that answers it. Python also just gets the bytes from the log envelope item seehttps://github.com/getsentry/sentry-python/blob/7787e6dcb7212b65aaabb1808795f1fed5abc7e9/sentry_sdk/transport.py#L307-L310
|
We also need a proper changelog entry for this |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks a lot for tackling this @tsushanth.
| - (void)recordLostLogBytes:(SentryEnvelopeItem *)envelopeItem reason:(SentryDiscardReason)reason | ||
| { | ||
| if ([SentryEnvelopeItemTypes.log isEqualToString:envelopeItem.type]) { | ||
| NSUInteger byteCount = envelopeItem.data.length; |
There was a problem hiding this comment.
The byte size does not need to be exact. SDKs MAY use an approximation, such as the in-memory size of the log object or the size of its serialized representation. The goal is to provide a useful signal, not a precise measurement. - develop docs
@philprime I think that answers it. Python also just gets the bytes from the log envelope item seehttps://github.com/getsentry/sentry-python/blob/7787e6dcb7212b65aaabb1808795f1fed5abc7e9/sentry_sdk/transport.py#L307-L310
| { | ||
| if ([SentryEnvelopeItemTypes.log isEqualToString:envelopeItem.type]) { | ||
| NSUInteger byteCount = envelopeItem.data.length; | ||
| if (byteCount > 0) { |
There was a problem hiding this comment.
l: No need for this check. Even if the byte count is 0 for whatever reason, we should still record it — that surfaces in Sentry that we dropped a log envelope item with no data, which normally shouldn't happen. This also matches the Python SDK, which records the byte count unconditionally.
| } | ||
| } | ||
|
|
||
| - (void)recordLostLogBytes:(SentryEnvelopeItem *)envelopeItem reason:(SentryDiscardReason)reason |
There was a problem hiding this comment.
h: We must add unit tests also for this new logic, please.
4 participants
Fixes #8175
The
log_bytedata category is defined in the client reports spec but was not tracked in sentry-cocoa. Onlylog_item(the item count) was reported; the byte-count counterpart was missing.What this changes
SentryDataCategory.h— addskSentryDataCategoryLogByte = 15and shiftskSentryDataCategoryUnknownto 16 to preserve the existing integer sequence.SentryDataCategoryMapper.h/.m— adds thekSentryDataCategoryNameLogByteconstant ("log_byte") and wires it intosentryDataCategoryForStringandnameForSentryDataCategory. The envelope-item-type mapper does not need a new branch becauselog_byteis a client-report category, not an envelope item type — thelogitem type already maps tolog_item.SentryHttpTransport.m— adds a private helperrecordLostLogBytes:reason:that recordsitem.data.lengthbytes underkSentryDataCategoryLogBytewhen a log envelope item is dropped. The helper is called alongside the existingrecordLostEvent/recordLostSpanscalls in three drop sites:envelopeItemDropped:withCategory:(rate-limit backoff)envelopeItemDeleted:withCategory:(cache overflow)recordLostEventFor:(send error)The
beforeSendLogpath inSentryClient.malready recordslog_itemfor the rejected log record. No byte count is available there because the log has not been serialized yet, solog_byteis intentionally not recorded at that site — this matches the behaviour of other SDKs that only count bytes at the transport boundary.SentryDataCategoryMapperTests.swift— extends the integer, string-to-category, and name-to-category round-trip tests to cover the newlogBytecase and updates the.unknowninteger assertion from 15 to 16.