[tracing] Migrate TraceLog observers to TraceSessionObserverList [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov added 3 comments![]( "Open in Gerrit")
if (sessions_after == 0 && tracing_sampling_timer_.IsRunning()) {This can continue tracing for longer than intended (e.g. a long-running trace session starts, then a short one with kTraceCategoryForPressureEvents starts and stops; sampling will continue until the end of the long-running session).
Should we plumb the session id to observers to correctly track which session stopped?
void RenderThreadImpl::OnTraceSessionStart(size_t sessions_before) {This will now emit extra events in the case of multiple sessions. Is this intentional?
if (sessions_after == 0) {ditto, this can trace past the end of the intended session.
Related details
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
if (sessions_after == 0 && tracing_sampling_timer_.IsRunning()) {This can continue tracing for longer than intended (e.g. a long-running trace session starts, then a short one with kTraceCategoryForPressureEvents starts and stops; sampling will continue until the end of the long-running session).
Should we plumb the session id to observers to correctly track which session stopped?
Discussed in chat, I implemented a "lazy" stop that checks the category in the timer.
I'm also exposing category API in perfetto so we can do proper tracking if needed: https://github.com/google/perfetto/pull/4106
void RenderThreadImpl::OnTraceSessionStart(size_t sessions_before) {This will now emit extra events in the case of multiple sessions. Is this intentional?
Actually I realize those should go away, I removed the updates here: https://chromium-review.googlesource.com/c/chromium/src/+/7147741
Because they are redundant, but forgot to remove the observer.
But in general yes, I think it's better to "restart" async events (given they don't nest) than missing them in a new tracing session.
ditto, this can trace past the end of the intended session.
In this case I'm optimistically removing the hook, this should be no worse than the status-quo.
Related details
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
1 comment
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
+dtapuska@ for content/ and t_p/blink/renderer
+fdoray@ for c/b/metrics
Related details
- Dave Tapuska
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
++active_track_event_sessions_;So could tracing be enabled before this observer is added? And cause active_trace_event_sessions_ to become negative?
It does not appear that OnStart is called if tracing is active when the listener is added (perhaps I missed it).
Related details
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
So could tracing be enabled before this observer is added? And cause active_trace_event_sessions_ to become negative?
It does not appear that OnStart is called if tracing is active when the listener is added (perhaps I missed it).
Good catch, I went with a different approach, instead of counting instance, I'm querying the state directly, accounting for OnStop instance with a helper.
Related details
- Dave Tapuska
- Francois Pierre Doray
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
4 comments
StopArgsImpl(const perfetto::DataSourceBase::StopArgs& args)Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
bool IsEnabledOnStop(uint32_t instances,Naming suggestion: `IsEnabledByInstance`?
bool enabled = false;Naming suggestion: `bool should_stop = true;`
Related details
- Dave Tapuska
- Etienne Pierre-Doray
- Francois Pierre Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
dtapuska is OOO -> +dgozman@ for content/ and t_p/b/renderer
StopArgsImpl(const perfetto::DataSourceBase::StopArgs& args)Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Done
bool IsEnabledOnStop(uint32_t instances,Naming suggestion: `IsEnabledByInstance`?
Not sure this is accurate: the function returns true if any instance is enabled, whereas IsEnabledByInstance() sounds like it returns the state only for a specific instance.
I updated the comment though.
Naming suggestion: `bool should_stop = true;`
- Dave Tapuska
- Dmitry Gozman
- Francois Pierre Doray
- Mikhail Khokhlov
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
- Dave Tapuska
- Dmitry Gozman
- Francois Pierre Doray
- Mikhail Khokhlov
- mmenke
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. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
1 comment
bool IsEnabledOnStop(uint32_t instances,Etienne Pierre-DorayNaming suggestion: `IsEnabledByInstance`?
Not sure this is accurate: the function returns true if any instance is enabled, whereas IsEnabledByInstance() sounds like it returns the state only for a specific instance.
I updated the comment though.
- Dave Tapuska
- Dmitry Gozman
- Etienne Pierre-Doray
- Francois Pierre Doray
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 5 comments![]( "Open in Gerrit")
[tracing] Migrate TraceLog observers to TraceSessionObserverListThis is confusing. Observers may be using `TraceEventSessionObserverList`, but they're becoming `TrackEventSessionObservers` (with a k). There should be some mention of this here.
void OnStart(const perfetto::DataSourceBase::StartArgs&) override;This API seems underspecified. I'm not sure what a "track event tracing session" is. Do these arguments represent an accumulation of global state, or more local state?
public perfetto::TrackEventSessionObserver {I'm confused by the CL description. It mentions migrating to TraceEventSessionObserver, but this is a TrackEventSessionObserver.
should_stop = !base::trace_event::IsEnabledOnStop(instances, args);So args are per-"trac[e|k] session", of which there can be more than one, while we need to apply that as a delta to the global tracing state (which I guess tracks number of active sessions, because OnStop is called before the global tracing state is updated).
This API seems incredibly clunky here. Is there a better way to be using it?
if (should_stop && net_log()) {We should probably just early return if !net_log()? Admittedly, perf doesn't matter here, it just seems weird to do the extra work before checking the class's own state.
Related details
- Dave Tapuska
- Dmitry Gozman
- Etienne Pierre-Doray
- Francois Pierre Doray
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. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Dave Tapuska
- Dmitry Gozman
- Etienne Pierre-Doray
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
[tracing] Migrate TraceLog observers to TraceSessionObserverListThis is confusing. Observers may be using `TraceEventSessionObserverList`, but they're becoming `TrackEventSessionObservers` (with a k). There should be some mention of this here.
For all intents and purposes, in Chrome TraceSession == TrackEventSession (TrackEvent is a data source that generates TRACE_EVENT data), that's because there's very little tracing that's not using TRACE_EVENT.
So the main thing this CL does is change TraceLog observers to a per-session observers.
For simplicity, I'm keeping TraceEventSessionObserver naming (I made the CL more consistent now).
void OnStart(const perfetto::DataSourceBase::StartArgs&) override;This API seems underspecified. I'm not sure what a "track event tracing session" is. Do these arguments represent an accumulation of global state, or more local state?
I added documentation in trace_session_observer.h
But the individual arguments are documented here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/include/perfetto/tracing/internal/track_event_internal.h;l=69?q=perfetto::TrackEventSessionObserver&ss=chromium
I'm confused by the CL description. It mentions migrating to TraceEventSessionObserver, but this is a TrackEventSessionObserver.
Fixed.
should_stop = !base::trace_event::IsEnabledOnStop(instances, args);So args are per-"trac[e|k] session", of which there can be more than one, while we need to apply that as a delta to the global tracing state (which I guess tracks number of active sessions, because OnStop is called before the global tracing state is updated).
This API seems incredibly clunky here. Is there a better way to be using it?
I improved the ergonomics a bit, now simply IsCategoryEnabledOnStop().
This API seems incredibly clunky here. Is there a better way to be using it?
It's a bit convoluted, I wanted to fix this in perfetto, but got push back
https://github.com/google/perfetto/pull/4096#issuecomment-3646238607
I also don't really want to make a "trace category" observer a first class citizen yet, because of the extra implementation complexity and similar use cases were better served by creating a custom data source
https://perfetto.dev/docs/instrumentation/tracing-sdk#custom-data-sources
Overall I wanted to prioritize the more common use case that emits state in OnStart(), and the less common "data source like" use case like this one (there's only 2 in chrome and 1 in v8) is retrofit in the same observer interface.
We should probably just early return if !net_log()? Admittedly, perf doesn't matter here, it just seems weird to do the extra work before checking the class's own state.
- Dave Tapuska
- Dmitry Gozman
- mmenke
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. |
mmenke (Gerrit)
mmenke voted and added 3 comments![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
3 comments
void OnStart(const perfetto::DataSourceBase::StartArgs&) override;Etienne Pierre-DorayThis API seems underspecified. I'm not sure what a "track event tracing session" is. Do these arguments represent an accumulation of global state, or more local state?
I added documentation in trace_session_observer.h
But the individual arguments are documented here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/include/perfetto/tracing/internal/track_event_internal.h;l=69?q=perfetto::TrackEventSessionObserver&ss=chromium
I saw that, but I don't know what a "track event tracing session" is. It could be e.g., a single consumer of trace events, or it could be a global singleton involving all consumers of trace events. I guess it's the former.
should_stop = !base::trace_event::IsEnabledOnStop(instances, args);Etienne Pierre-DoraySo args are per-"trac[e|k] session", of which there can be more than one, while we need to apply that as a delta to the global tracing state (which I guess tracks number of active sessions, because OnStop is called before the global tracing state is updated).
This API seems incredibly clunky here. Is there a better way to be using it?
I improved the ergonomics a bit, now simply IsCategoryEnabledOnStop().
This API seems incredibly clunky here. Is there a better way to be using it?
It's a bit convoluted, I wanted to fix this in perfetto, but got push back
https://github.com/google/perfetto/pull/4096#issuecomment-3646238607I also don't really want to make a "trace category" observer a first class citizen yet, because of the extra implementation complexity and similar use cases were better served by creating a custom data source
https://perfetto.dev/docs/instrumentation/tracing-sdk#custom-data-sourcesOverall I wanted to prioritize the more common use case that emits state in OnStart(), and the less common "data source like" use case like this one (there's only 2 in chrome and 1 in v8) is retrofit in the same observer interface.
- Dave Tapuska
- Dmitry Gozman
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 1 comment![]( "Open in Gerrit")
Sorry, I am OOO until January. Moving myself from reviewers to cc.
Related details
- Dave Tapuska
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
- Dave Tapuska
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
+chromium-metrics-reviews@ for c/b/metrics
+a...@chromium.org for content/app
Related details
- Avi Drissman
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
rka...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Avi Drissman
- Dave Tapuska
- Robert Kaplow
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Etienne Pierre-Doray
- Robert Kaplow
Code-Coverage
Code-Owners
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
- Dave Tapuska
- Robert Kaplow
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[tracing] Migrate TraceLog observers to TraceSessionObserverList
This CL implements TraceSessionObserverList and migrate
AsyncEnabledStateObserver/EnabledStateObserver to it.
The main difference is that TraceSessionObserver is a per-session
observer.
This is using ObserverListThreadSafe which makes the implementation trivial.
Reason:
- TraceLog observer doesn't support multiple sessions (legacy reason).
This can cause missing updates for several observers that would need it.
- Unlike TrackEvent::AddSessionObserver, TraceSessionObserverList sends
notification on the task runner where it was requested.
- This merges AsyncEnabledStateObserver/EnabledStateObserver;
essentially there's no good use case for non async version; otherwise
TrackEventSessionObserver can be used directly.
- Instead of OnTraceLogEnabled(), this is replaced by
TRACE_EVENT_CATEGORY_ENABLED with the relevant category.
- One step towards making TraceLog test only.
- M base/BUILD.gn
- M base/test/trace_test_utils.cc
- M base/trace_event/trace_event_unittest.cc
- M base/trace_event/trace_log.cc
- M base/trace_event/trace_log.h
- A base/trace_event/trace_session_observer.cc
- A base/trace_event/trace_session_observer.h
- M chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
- M chrome/browser/metrics/pressure/pressure_metrics_reporter.cc
- M chrome/browser/metrics/pressure/pressure_metrics_reporter.h
- M components/power_metrics/system_power_monitor.cc
- M components/power_metrics/system_power_monitor.h
- M components/power_metrics/system_power_monitor_unittest.cc
- M content/app/content_main.cc
- M content/renderer/render_thread_impl.cc
- M content/renderer/render_thread_impl.h
- M net/log/trace_net_log_observer.cc
- M net/log/trace_net_log_observer.h
- M services/tracing/public/cpp/trace_startup.cc
- M third_party/blink/renderer/core/inspector/worker_inspector_controller.cc
- M third_party/blink/renderer/core/inspector/worker_inspector_controller.h
- M third_party/blink/renderer/core/paint/timing/paint_timing_visualizer.cc
- M third_party/blink/renderer/core/paint/timing/paint_timing_visualizer.h
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.h
- M third_party/blink/renderer/platform/instrumentation/tracing/trace_event.cc
- M third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Code-Review: +1 by Kentaro Hara, +1 by Francois Pierre Doray, +1 by Mikhail Khokhlov, +1 by Avi Drissman, +1 by mmenke
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |