Consolidate GC logs on boot into ContainerLoadStats event#26748
Consolidate GC logs on boot into ContainerLoadStats event#26748markfields wants to merge 4 commits intomicrosoft:mainfrom
Conversation
(and get rid of redundant logging props - it's all covered by gcConfig and options)
|
|
||
| private readonly configs: IGarbageCollectorConfigs; | ||
|
|
||
| public readonly serializedConfigs: string; |
There was a problem hiding this comment.
Might be simpler to expose the configs and have it serialized at logging time in Container Runtime? That way we don't have to track the same config twice here.
There was a problem hiding this comment.
It's to prevent tampering with the configs. I didn't like exposing the config object directly
There was a problem hiding this comment.
I guess downside of this is there's a bit of persistent memory bloat. Maybe not worth it, I guess I don't see someone having a reason to modify it or accidentally doing it. What do you think?
There was a problem hiding this comment.
Make it a getter and readonly?
There was a problem hiding this comment.
Yeah all the props are already readonly - it's pedantic but there's some nested objects in there which could be mucked with.
How about I switch serializedConfig to a getter that does the JSON.stringify, rather than storing the string. Since we know it's only called once.
There was a problem hiding this comment.
Pull request overview
This PR reduces boot-time telemetry event volume by removing GC-specific boot events and folding their data into the existing ContainerLoadStats event.
Changes:
- Added
IGarbageCollector.serializedConfigsfor emitting GC config telemetry without re-serializing at each call site. - Removed the
GarbageCollectorLoadedandGCFeatureMatrixtelemetry events. - Added
gcConfigsto theContainerLoadStatstelemetry payload.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/runtime/container-runtime/src/gc/gcDefinitions.ts | Extends IGarbageCollector with a serialized GC-configs telemetry accessor. |
| packages/runtime/container-runtime/src/gc/garbageCollection.ts | Implements serializedConfigs and removes the GarbageCollectorLoaded boot event. |
| packages/runtime/container-runtime/src/containerRuntime.ts | Removes GCFeatureMatrix event and adds gcConfigs to ContainerLoadStats. |
You can also share your feedback on Copilot code review. Take the survey.
Description
We're trying to reduce logging overhead/noise from high event volume for consumers. These two events occur on boot and the info can be consolidated into the canonical boot event "ContainerLoadStats":