ref(core): Switch to standardized log envelope #16133
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AbhiPrasad
requested review from
lforst and
Lms24
and removed request for
a team
AbhiPrasad
commented
Apr 24, 2025
|
|
||
| // Clear the log buffer after envelopes have been constructed. | ||
| logBuffer.length = 0; | ||
| CLIENT_TO_LOG_BUFFER_MAP.set(client, []); |
There was a problem hiding this comment.
This was an actual bug fix, because we mutated the reference, it has the possibility to cause weird side effects.
size-limit report 📦
|
Lms24
approved these changes
Apr 25, 2025
Comment on lines
+140
to
+143
| timestamp: timestampInSeconds(), | ||
| level, | ||
| body: message, | ||
| trace_id: traceContext?.trace_id, | ||
| severity_number: severityNumber ?? SEVERITY_TEXT_TO_SEVERITY_NUMBER[level], |
There was a problem hiding this comment.
q: are we doing any kind of validation on level and severityNumber if they match? is there some kind of precedence one has over the other?
There was a problem hiding this comment.
The passed in severityNumber takes priority, but there's no validation (so users can technically pass in whatever if they ignore the type check). Relay should handle normalizing all of this though, so we don't need to worry too much about what gets passed in.
| * @param logs - The logs to include in the envelope. | ||
| * @param metadata - The metadata to include in the envelope. | ||
| * @param tunnel - The tunnel to include in the envelope. | ||
| * @param dsn - The DSN to include in the envelope. | ||
| * @returns The created envelope. | ||
| */ | ||
| export function createOtelLogEnvelope( | ||
| logs: Array<SerializedOtelLog>, | ||
| export function createLogEnvelope( |
There was a problem hiding this comment.
l: just to confirm - log envelop headers don't need to contain the trace header?
There was a problem hiding this comment.
yup - more info about this here: https://www.notion.so/sentry/Log-Envelopes-Dynamic-Sampling-1d68b10e4b5d8061a035dc8bc5057196
AbhiPrasad
force-pushed
the
abhi-use-new-log-format
branch
from
124f532 to
3d5d500
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To test logs for the alpha we used a standardized
otel_logenvelope item.Now that the logs protocol is more stable, we are switching to the sentry specific envelope item. This is fully documented here: https://develop.sentry.dev/sdk/telemetry/logs/