feat: store memory footprints to grafana #9001
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
@sjaanus, core features have been modified in this pull request. Please review carefully! |
github-actions
bot
requested review from
chriswk,
FredrikOseberg,
gastonfournier and
ivarconr
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
| @@ -69,6 +70,8 @@ export default class FeatureController extends Controller { | |||
|
|
|||
| private eventBus: EventEmitter; | |||
|
|
|||
| private clientFeaturesCacheMap = new Map<string, number>(); | |||
There was a problem hiding this comment.
We can not read the size of memoizee directly, so we will be storing the memory usage for each query key into new cache. This will be calculated on new revision id.
| @@ -162,6 +165,32 @@ export default class FeatureController extends Controller { | |||
| private async resolveFeaturesAndSegments( | |||
| query?: IFeatureToggleQuery, | |||
| ): Promise<[FeatureConfigurationClient[], IClientSegment[]]> { | |||
| if (this.flagResolver.isEnabled('deltaApi')) { | |||
There was a problem hiding this comment.
Only if flag on, do this logic, otherwise go default. We will start very gradual rollout.
| featuresSize + segmentsSize, | ||
| ); | ||
|
|
||
| await this.clientFeatureToggleService.getClientDelta( |
There was a problem hiding this comment.
We are just making the same call in delta API to request for cache update.
| try { | ||
| const featuresSize = this.getCacheSizeInBytes(features); | ||
| const segmentsSize = this.getCacheSizeInBytes(segments); | ||
| this.clientFeaturesCacheMap.set( |
There was a problem hiding this comment.
Saving the current key into cache.
There was a problem hiding this comment.
Again: can query be undefined here (because we force it with ! later)?
So is this the size of the query without delta or with?
There was a problem hiding this comment.
Ah, I see now. This is specified in the store footprint method. This is for features.
| ); | ||
| this.storeFootprint(); | ||
| } catch (e) { | ||
| this.logger.error('Delta diff failed', e); |
There was a problem hiding this comment.
Putting all in try catch if something fails for extra safety.
|
|
||
| await this.clientFeatureToggleService.getClientDelta( | ||
| undefined, | ||
| query!, |
There was a problem hiding this comment.
When will query not be present? This ! is a little icky
| try { | ||
| const featuresSize = this.getCacheSizeInBytes(features); | ||
| const segmentsSize = this.getCacheSizeInBytes(segments); | ||
| this.clientFeaturesCacheMap.set( |
There was a problem hiding this comment.
Again: can query be undefined here (because we force it with ! later)?
So is this the size of the query without delta or with?
| try { | ||
| const featuresSize = this.getCacheSizeInBytes(features); | ||
| const segmentsSize = this.getCacheSizeInBytes(segments); | ||
| this.clientFeaturesCacheMap.set( |
There was a problem hiding this comment.
Ah, I see now. This is specified in the store footprint method. This is for features.
|
|
||
| storeFootprint() { | ||
| try { | ||
| const featuresMemory = this.getCacheSizeInBytes(this.delta); |
There was a problem hiding this comment.
How many of these do we keep at the same time? Help me wrap my head around this: can the delta be different for each connected client (assuming they're polling and you're making a flag change every second)?
presumably this uses the revision id to store something, right, so it doesn't need that. I ... remember you saying something about that.
There was a problem hiding this comment.
We keep a delta for each environment. So all clients that connect from same environment, share it.
When there is new revision, we will start storing memory footprint for old client-api and the new delta-api.
We will be sending it as prometheus metrics.
The memory size will only be recalculated if revision changes, which does not happen very often.