Keep Newest Breadcrumbs Instead of Oldest #858
Merged
+11
−2
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
dajinchu
commented
Feb 7, 2025
| @@ -39,6 +39,16 @@ defmodule Sentry.ContextTest do | |||
| assert first_breadcrumb.timestamp <= second_breadcrumb.timestamp | |||
| end | |||
|
|
|||
| test "retains most recent breadcrumbs when exceeding max" do | |||
There was a problem hiding this comment.
This test fails on master.
|
@dajinchu thanks for the PR! What do other SDKs do, like the Python or Ruby ones? 🙃 |
|
@whatyouhide Javascript lib adds crumbs to the end of the list, and slices from the end of the list, thus retaining the most recent. |
whatyouhide
reviewed
Feb 9, 2025
lib/sentry/context.ex
Outdated
Comment on lines
426
to
427
| breadcrumbs = [map | breadcrumbs] | ||
| Enum.take(breadcrumbs, -1 * Sentry.Config.max_breadcrumbs()) | ||
| Enum.take(breadcrumbs, 1 * Sentry.Config.max_breadcrumbs()) |
There was a problem hiding this comment.
Can you maybe rewrite this code to
Enum.take([map | breadcrumbs], Sentry.Config.max_breadcrumbs())?
There was a problem hiding this comment.
yeah haha what was I even thinking
whatyouhide
approved these changes
Feb 10, 2025
|
Thank you @dajinchu 💟 |
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.
Currently, exceeding max_breadcrumbs causes only the oldest breadcrumbs to be retained, instead of the newest, as I would have expected. I believe this is a bug?
It's caused by the following code, which inserts the newest breadcrumb at the front of the list, but then trims the list from the back.
sentry-elixir/lib/sentry/context.ex
Lines 426 to 427 in 8ee657f
Similar code exists here but is actually correct because
Context.get_allreturns the breadcrumb list reversed.sentry-elixir/lib/sentry/event.ex
Line 216 in 8ee657f