Skip to content

improved maxBreadcrumbs description #4682

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

improved maxBreadcrumbs description #4682

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/types/src/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ export interface Options {
dist?: string;

/**
* The maximum number of breadcrumbs sent with events. Defaults to 100.
* Values over 100 will be ignored and 100 used instead.
* The maximum number of breadcrumbs sent with events. Defaults to 100,
* but you can set this to any number. However, you should be aware that
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite true. See:

sentry-javascript/packages/hub/src/scope.ts

Line 370 in 2cf6f94

const maxCrumbs = typeof maxBreadcrumbs === 'number' ? Math.min(maxBreadcrumbs, MAX_BREADCRUMBS) : MAX_BREADCRUMBS;

100 is effectively a ceiling on the value, it’s a hard limit. Users can set a value <= 100, but all values over a 100 will become a 100.

I’d be fine to add the max payload note, but I’m open to just making the above more clear in the docs.

Copy link
Author

@imatwawana imatwawana Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am confusion, lol. And maybe this is a difference between SDKs. The user was setting up React Native, but in their IDE, this was the note that popped up above this option (maybe 'cause RN imports stuff from React which is JS?). But for RN I've been told this isn't limited. The linked issue has the full discussion.

Copy link
Author

Choose a reason for hiding this comment

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

Tagging @marandaneto

Copy link
Member

Choose a reason for hiding this comment

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

How about:

   * The maximum number of breadcrumbs sent with events. Defaults to 100.
   * Values of this option over 100 will be ignored and 100 used instead.

The payload stuff is true, but lots of things can make the payload too big, not just breadcrumbs.

I would think that if RN has a different max (or no max), they'd be able to just override the type with one of their own. (Left a comment to a similar effect on the issue.)

Copy link

Choose a reason for hiding this comment

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

Ok, looks like we have a different implementation across the SDKs, for all the Mobile SDKs, the maxBreadcrumbs is an open number, if it's given, it should be respected, it does not matter how big it is, e.g. you may have 1 breadcrumb with huge metadata which fills up the payload size, but you can also have a 1000 breadcrumbs, which wouldn't hurt the payload at all, it really depends on usage, so I was not aware of this absolute maxBreadcrumbs.
Sorry for the confusion @imatwawana

I'll bring this up to my team and see if it makes sense to have such absolute maxBreadcrumbs and then we adjust our RN (source-code) docs.
The product docs cannot reflect one way or the other since SDKs do differently, I'll clear this out first before going ahead with docs change.

Copy link
Author

Choose a reason for hiding this comment

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

Shall I go ahead and close this PR then?

Copy link
Author

Choose a reason for hiding this comment

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

How about:

   * The maximum number of breadcrumbs sent with events. Defaults to 100.
   * Values of this option over 100 will be ignored and 100 used instead.

The payload stuff is true, but lots of things can make the payload too big, not just breadcrumbs.

I would think that if RN has a different max (or no max), they'd be able to just override the type with one of their own. (Left a comment to a similar effect on the issue.)

I think we can leave the text as is, because it means what it says. The confusion was simply because this was showing up in the RN source code docs, but wasn't matching the behaviour or other source code documentation in a sample RN app.

Copy link
Member

Choose a reason for hiding this comment

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

Great, then I will close this PR.

* Sentry has a maximum payload size of 1MB and any events exceeding that
* payload size will be dropped.
*/
maxBreadcrumbs?: number;

Expand Down