improved maxBreadcrumbs description

[imatwawana]

Resolves 4496

[AbhiPrasad]

First PR in the sdk? I sense a promising future :0

[AbhiPrasad]

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.

[imatwawana]

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.

[imatwawana]

Tagging @marandaneto

[lobsterkatie]

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.)

[marandaneto]

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.

[imatwawana]

Shall I go ahead and close this PR then?

[imatwawana]

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.

[lobsterkatie]

Great, then I will close this PR.

[lobsterkatie]

Closing because the current description is accurate. If changes need to be made, it needs to be in docs or in the RN SDK, as per getsentry/sentry-docs#4496 (comment).