Add runtime safety guards for early system profile recording. [chromium/src : main]
Luc Nguyen (Gerrit)
Luc Nguyen added 1 comment![]( "Open in Gerrit")
Luc NguyenSorry, jetski started pushing to this CL without permission and I lost your +1s. There is no difference between the Patchset 16 that you approved and newest Patchset 20
I think it happened again?
Related details
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alicja Opalinska (Gerrit)
Alicja Opalinska added 1 comment![]( "Open in Gerrit")
Luc NguyenSorry, jetski started pushing to this CL without permission and I lost your +1s. There is no difference between the Patchset 16 that you approved and newest Patchset 20
I think it happened again?
ughh this time it was me creating a new branch and trying to make a new CL and still it got here..
Related details
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen added 1 comment![]( "Open in Gerrit")
sorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
Related details
- Alexei Svitkine
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alicja Opalinska (Gerrit)
Alicja Opalinska added 1 comment![]( "Open in Gerrit")
sorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
This came up in the discussion in the design doc, we want to make sure that the 'early safe' providers remain 'early safe' and that down the road we don't introduce logging histograms or blocking I/O into this phase. Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data. if not that then there is also risk of double counting
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen added 1 comment![]( "Open in Gerrit")
Alicja Opalinskasorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
This came up in the discussion in the design doc, we want to make sure that the 'early safe' providers remain 'early safe' and that down the road we don't introduce logging histograms or blocking I/O into this phase. Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data. if not that then there is also risk of double counting
Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data
hmm not sure i agree. i don't think there ever has been a phase where you couldn't emit histograms? e.g. there are histograms that get emitted much earlier than this, e.g. to measure start up performances
i guess the part i'm not sure i follow is, why would emitting a histogram mean it's not a safe provider anymore? what if there are "unrelated" code paths that emit code paths? e.g. say i have a provider that calls some unrelated helper like `base::GetCurrentTime()` to get the current time, and say that helper had a histogram that timed the runtime of the function, that's not allowed?
hmm, maybe you are talking specifically about histograms that our serverside pipelines use to derive fields?
Related details
- Alexei Svitkine
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alicja Opalinska (Gerrit)
Alicja Opalinska added 1 comment![]( "Open in Gerrit")
Alicja Opalinskasorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
Luc NguyenThis came up in the discussion in the design doc, we want to make sure that the 'early safe' providers remain 'early safe' and that down the road we don't introduce logging histograms or blocking I/O into this phase. Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data. if not that then there is also risk of double counting
Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data
hmm not sure i agree. i don't think there ever has been a phase where you couldn't emit histograms? e.g. there are histograms that get emitted much earlier than this, e.g. to measure start up performances
i guess the part i'm not sure i follow is, why would emitting a histogram mean it's not a safe provider anymore? what if there are "unrelated" code paths that emit code paths? e.g. say i have a provider that calls some unrelated helper like `base::GetCurrentTime()` to get the current time, and say that helper had a histogram that timed the runtime of the function, that's not allowed?
hmm, maybe you are talking specifically about histograms that our serverside pipelines use to derive fields?
I think I see your point, I didn't realize the connection earlier to other unrelated helpers. The DCHECK idea was briefly discussed in the design doc, but I agree its maybe too aggressive.
Do you agree though that we still need to ensure 'early safe' providers don't accidentally perform blocking disk I/O on the main thread? and that we need to prevent double logging of some status histograms?
I can just have specific early providers check IsPopulatingEarlyProfile() flag and skip UMA_HISTOGRAM calls, keep the ScopedDisallowBlocking guard
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
Alicja Opalinskasorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
Luc NguyenThis came up in the discussion in the design doc, we want to make sure that the 'early safe' providers remain 'early safe' and that down the road we don't introduce logging histograms or blocking I/O into this phase. Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data. if not that then there is also risk of double counting
Alicja OpalinskaLogging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data
hmm not sure i agree. i don't think there ever has been a phase where you couldn't emit histograms? e.g. there are histograms that get emitted much earlier than this, e.g. to measure start up performances
i guess the part i'm not sure i follow is, why would emitting a histogram mean it's not a safe provider anymore? what if there are "unrelated" code paths that emit code paths? e.g. say i have a provider that calls some unrelated helper like `base::GetCurrentTime()` to get the current time, and say that helper had a histogram that timed the runtime of the function, that's not allowed?
hmm, maybe you are talking specifically about histograms that our serverside pipelines use to derive fields?
I think I see your point, I didn't realize the connection earlier to other unrelated helpers. The DCHECK idea was briefly discussed in the design doc, but I agree its maybe too aggressive.
Do you agree though that we still need to ensure 'early safe' providers don't accidentally perform blocking disk I/O on the main thread? and that we need to prevent double logging of some status histograms?
I can just have specific early providers check IsPopulatingEarlyProfile() flag and skip UMA_HISTOGRAM calls, keep the ScopedDisallowBlocking guard
We don't care so much about incidental histograms.
What we want to avoid is histograms that we expect to be logged once per log, to be logged multiple times per log due to the provider running at startup and then later also.
Related details
- Alicja Opalinska
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alicja Opalinska (Gerrit)
Alicja Opalinska added 1 comment![]( "Open in Gerrit")
Alicja Opalinskasorry, i'm not sure i understand (the CL description explains what it's doing but not why) -- i.e., why would we want to prevent emitting histograms? what would go wrong if we did emit histograms?
Luc NguyenThis came up in the discussion in the design doc, we want to make sure that the 'early safe' providers remain 'early safe' and that down the road we don't introduce logging histograms or blocking I/O into this phase. Logging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data. if not that then there is also risk of double counting
Alicja OpalinskaLogging during this early phase outside the normal metrics logging flow, where not everything is initialized yet can corrupt the data
hmm not sure i agree. i don't think there ever has been a phase where you couldn't emit histograms? e.g. there are histograms that get emitted much earlier than this, e.g. to measure start up performances
i guess the part i'm not sure i follow is, why would emitting a histogram mean it's not a safe provider anymore? what if there are "unrelated" code paths that emit code paths? e.g. say i have a provider that calls some unrelated helper like `base::GetCurrentTime()` to get the current time, and say that helper had a histogram that timed the runtime of the function, that's not allowed?
hmm, maybe you are talking specifically about histograms that our serverside pipelines use to derive fields?
Alexei SvitkineI think I see your point, I didn't realize the connection earlier to other unrelated helpers. The DCHECK idea was briefly discussed in the design doc, but I agree its maybe too aggressive.
Do you agree though that we still need to ensure 'early safe' providers don't accidentally perform blocking disk I/O on the main thread? and that we need to prevent double logging of some status histograms?
I can just have specific early providers check IsPopulatingEarlyProfile() flag and skip UMA_HISTOGRAM calls, keep the ScopedDisallowBlocking guard
We don't care so much about incidental histograms.
What we want to avoid is histograms that we expect to be logged once per log, to be logged multiple times per log due to the provider running at startup and then later also.
Alright, so removing the dchecks, adding a flag IsEarlyMetricsRecordingActive so that the early providers will be able to check before emitting a once per log histogram. This is not yet used anywhere as at the moment we dont have any UMA_HISTOGRAM calls in the ProvideSystemProfileMetrics of these early providers, but we might in the future, for example in GPUMetricsProvider in the next phase.
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |