sdk-core: reduce breadcrumbs size #228
Conversation
packages/sdk-core/src/modules/breadcrumbs/model/BreadcrumbLimits.ts
Outdated
Show resolved
Hide resolved
packages/sdk-core/src/modules/breadcrumbs/storage/InMemoryBreadcrumbsStorage.ts
Outdated
Show resolved
Hide resolved
packages/sdk-core/src/modules/breadcrumbs/storage/InMemoryBreadcrumbsStorage.ts
Outdated
Show resolved
Hide resolved
perf2711
force-pushed
the
feature/BT-718-reduce-breadcrumb-size
branch
from
6c41841 to
5b03f4d
Compare
packages/sdk-core/src/modules/breadcrumbs/BreadcrumbsManager.ts
Outdated
Show resolved
Hide resolved
| @@ -53,6 +60,33 @@ export class InMemoryBreadcrumbsStorage implements BreadcrumbsStorage, Backtrace | |||
|
|
|||
| this._breadcrumbs.add(breadcrumb); | |||
|
|
|||
| if (this._limits.maximumBreadcrumbsSize) { | |||
There was a problem hiding this comment.
we're applying this rule twice. In the BreadcrumbsManager L:194 and here. Maybe we should always expect the storage to check it. I think it might make more sense if we assume the breadcrumb that exceed the limit will be dropped.
There was a problem hiding this comment.
it's not applied twice, in BreadcrumbManager maximumBreadcrumbSize is checked, and here maximumBreadcrumbsSize. I'll rename the latter to match BreadcrumbLimit.maximumTotalBreadcrumbsSize
There was a problem hiding this comment.
later you mean in another pull request?
There was a problem hiding this comment.
latter, not later.
| } | ||
|
|
||
| let limitedBreadcrumb: RawBreadcrumb | LimitedRawBreadcrumb; | ||
| if (this._limits.maximumAttributesDepth !== undefined && rawBreadcrumb.attributes) { |
There was a problem hiding this comment.
wouldn't it be better if we modify limitedBreadcurmb variable instead?
There was a problem hiding this comment.
nope, it isn't initialized at this point
There was a problem hiding this comment.
sorry my bad. I meant rawBreadcrumb instead of limitedBreadcrumb. WE don't need to use limitedBreadcrumb at all
There was a problem hiding this comment.
no, because _interceptor above expects RawBreadcrumb and not LimitedRawBreadcrumb
perf2711
force-pushed
the
feature/BT-718-reduce-breadcrumb-size
branch
from
3431f57 to
84f390a
Compare
| } | ||
|
|
||
| let limitedBreadcrumb: RawBreadcrumb | LimitedRawBreadcrumb; | ||
| if (this._limits.maximumAttributesDepth !== undefined && rawBreadcrumb.attributes) { |
There was a problem hiding this comment.
sorry my bad. I meant rawBreadcrumb instead of limitedBreadcrumb. WE don't need to use limitedBreadcrumb at all
… and level/type checks
perf2711
force-pushed
the
feature/BT-718-reduce-breadcrumb-size
branch
from
84f390a to
8d4594f
Compare
perf2711
changed the title
This PR adds limits for controlling breadcrumb file sizes:
maximumAttributesDepthmaximumBreadcrumbMessageLengthmaximumBreadcrumbSizemaximumBreadcrumbsSizeIt also allows to disable all breadcrumb limits by setting
falseto them.Platform-specific changes will be included in subsequent pull requests.