Introducing new max for foreground periods

[ThibautGeriz]

Motivation

We have quite a log of Reached maximum of foreground time errors but the views do not have 500 items.

Changes

Let's split the max in two:

• a max of periods stored to limit memory used
• a max of periods by view to limit bandwidth

Testing

It's unit tested

[codecov-commenter]

Codecov Report

Merging #1032 (2e4e8bc) into main (b3bae33) will increase coverage by 0.00%.
The diff coverage is 81.81%.

@@           Coverage Diff           @@
##             main    #1032   +/-   ##
=======================================
  Coverage   88.93%   88.94%           
=======================================
  Files          88       88           
  Lines        4168     4170    +2     
  Branches      959      959           
=======================================
+ Hits         3707     3709    +2     
  Misses        461      461           

Impacted Files	Coverage Δ
.../domain/rumEventsCollection/view/viewCollection.ts	100.00% <ø> (ø)
packages/rum-core/src/domain/foregroundContexts.ts	85.52% <77.77%> (+0.39%)	⬆️
...ain/rumEventsCollection/action/actionCollection.ts	95.23% <100.00%> (ø)
...omain/rumEventsCollection/error/errorCollection.ts	76.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3bae33...2e4e8bc. Read the comment docs.

[bcaudan]

Do we need to keep and increase MAX_NUMBER_OF_FOCUSED_TIME?

[BenoitZugmeyer]

The idea is to have two limits serving two different purposes:

• MAX_NUMBER_OF_FOCUSED_TIME is here to limit the memory usage. It can be fairly large as objects are small.
• MAX_NUMBER_OF_FOCUSED_TIME_PER_VIEW is here to limit the bandwidth usage. We keep the same limit as before (500) as it should be enough.

[ThibautGeriz]

Agreed. Most of the views have less than 500 focused periods therefore the array is full because of SPA changes. We can drastically reduce the number of errors while keeping the same limit for the bandwidth.

[bcaudan]

IIUC:

• MAX_NUMBER_OF_FOCUSED_TIME is the max number of foreground periods stored
• MAX_NUMBER_OF_FOCUSED_TIME_PER_VIEW is the max number of foreground periods than can be returned by getInForegroundPeriods.

if that so, we could probably clarify the namings:

• using "view" in the constant is "leaking" information about the current consumer which we don't need to know here
• I am confused by "focused time" vs "foreground period", is there a difference?
• we could maybe be more expressive than get for getInForeground and getInForegroundPeriods and use the same verb for getInForegroundPeriods and the related constant.

Given that, I would have in mind something like:

const MAX_NUMBER_OF_SELECTABLE_FOREGROUND_PERIODS = 500
const MAX_NUMBER_OF_STORED_FOREGROUND_PERIODS = 2500

isInForegroundAt: (startTime: RelativeTime) => boolean | undefined
selectInForegroundPeriodsFor: (startTime: RelativeTime, duration: Duration) => InForegroundPeriod[] | undefined

wdyt?

[ThibautGeriz]

Agreed, the naming you're proposing is better than the current one

[bcaudan]

LGTM