[tracing] Forward startup tracing config as shmem [chromium/src : main]
Eric Seckler (Gerrit)
Eric Seckler added 1 comment![]( "Open in Gerrit")
Implement early startup background tracingPlease add a little more context 😊
Related details
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler voted and added 11 comments![]( "Open in Gerrit")
Votes added by Eric Seckler
| Code-Review | +1 |
11 comments
Very very cool! Thanks Etienne! 😊
Added khokhlov@ for additional review since I think he might remember more of the caveats of past startup tracing issues.
if (child_fds[content::ZygoteForkDelegate::kTracingFDIndex].is_valid()) {kTraceConfigFDIndex
// |child_fds| should contain either kNumPassedFDs or kNumPassedFDs-1 fileNeeds updating if you are changing the condition below. Not quite clear why we are changing it though?
TRACING_EXPORT extern const char kTraceStartupEnablePrivacyFiltering[];Maybe we can get rid of this one now?
TRACING_EXPORT extern const char kTraceStartupRecordMode[];Wondering if there are any real users of this left now.
const char kTraceConfigHandle[] = "trace-config-handle";Can we add a comment? 😊
g_fds->Set(kTracingSharedMemoryDescriptor,kTraceConfigShared... (so it's clear this is different from the SMB used for actual tracing between producers & service)
OverrideFrameworkBundlePath();Not sure what these are used for, maybe deserves a comment? (or mention in the change description)
// child to begin tracing right away via startup tracing command line flags.nit: update
trace_config = startup_config.GetPerfettoConfig();Is this guaranteed to have the privacy filtering flag set in the config if the session owner is background tracing?
Btw, we discovered today that we probably shouldn't propagate the config when the owner of the current TraceLog session is system tracing... (unless we set --startup-tracing-owner accordingly and verify that this would then actually work correctly)
Mikhail was going to look into a fix for this.
Related details
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov added 4 comments![]( "Open in Gerrit")
// Startup tracing shared memory region config.nit: Startup tracing config shared memory region.
perfetto::DataSourceConfig data_source_config =
trace_log->GetCurrentTrackEventDataSourceConfig();
if (data_source_config.has_interceptor_config()) {
return base::ReadOnlySharedMemoryRegion();
}
*trace_config.add_data_sources()->mutable_config() = data_source_config;I suggest the following implementation (see the comment below for rationale):
```
bool has_relevant_config = false;
for (const auto& session : trace_log->GetTrackEventSessions()) {
if (session.backend == perfetto::kCustomBackend &&
!session.config.has_interceptor_config()) {
*trace_config.add_data_sources()->mutable_config() = session.config;
has_relevant_config = true;
break;
}
}
if (!has_relevant_config) {
base::ReadOnlySharedMemoryRegion();
}
```
Btw, we discovered today that we probably shouldn't propagate the config when the owner of the current TraceLog session is system tracing... (unless we set --startup-tracing-owner accordingly and verify that this would then actually work correctly)
Mikhail was going to look into a fix for this.
Right now --trace-startup-owner doesn't affect backend selection; we always start startup tracing with custom backend [1]. The fix itself is relatively straightforward, but would require extensive testing. For the purpose of this CL, I suggest just skipping system tracing sessions.
Another thing to keep in mind is that there can be multiple tracing sessions active at the same time. But since we skip system sessions anyway, and having multiple custom backend sessions is unlikely, I think something like "propagate the first non-system session" logic should be fine.
"benchmark", "toplevel", "startup", "disabled-by-default-file",nit: something's wrong with formatting?
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 14 comments![]( "Open in Gerrit")
Implement early startup background tracingPlease add a little more context 😊
Done
if (child_fds[content::ZygoteForkDelegate::kTracingFDIndex].is_valid()) {Etienne Pierre-DoraykTraceConfigFDIndex
Done
// |child_fds| should contain either kNumPassedFDs or kNumPassedFDs-1 fileNeeds updating if you are changing the condition below. Not quite clear why we are changing it though?
Done
TRACING_EXPORT extern const char kTraceStartupEnablePrivacyFiltering[];Maybe we can get rid of this one now?
Done
TRACING_EXPORT extern const char kTraceStartupRecordMode[];Wondering if there are any real users of this left now.
The equivalent of this is used in config file (in telemetry), so maybe it makes sense to keep.
const char kTraceConfigHandle[] = "trace-config-handle";Can we add a comment? 😊
Done
kTraceConfigShared... (so it's clear this is different from the SMB used for actual tracing between producers & service)
Done
// Startup tracing shared memory region config.nit: Startup tracing config shared memory region.
Done
Not sure what these are used for, maybe deserves a comment? (or mention in the change description)
Done
// child to begin tracing right away via startup tracing command line flags.Etienne Pierre-Doraynit: update
Done
Is this guaranteed to have the privacy filtering flag set in the config if the session owner is background tracing?
There's no explicit check, but GetDefaultBackgroundStartupConfig() unconditionally set privacy filter to true (it's only code path that sets kBackgroundTracing session owner), so I'd say yes.
perfetto::DataSourceConfig data_source_config =
trace_log->GetCurrentTrackEventDataSourceConfig();
if (data_source_config.has_interceptor_config()) {
return base::ReadOnlySharedMemoryRegion();
}
*trace_config.add_data_sources()->mutable_config() = data_source_config;I suggest the following implementation (see the comment below for rationale):
```
bool has_relevant_config = false;
for (const auto& session : trace_log->GetTrackEventSessions()) {
if (session.backend == perfetto::kCustomBackend &&
!session.config.has_interceptor_config()) {
*trace_config.add_data_sources()->mutable_config() = session.config;
has_relevant_config = true;
break;
}
}
if (!has_relevant_config) {
base::ReadOnlySharedMemoryRegion();
}
```
Done
"benchmark", "toplevel", "startup", "disabled-by-default-file",nit: something's wrong with formatting?
I tried fixing and git cl format is fighting me. Not sure how it gets to this result, but the content seems ok.
Related details
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| 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")
Related details
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
- Eric Seckler
- Etienne Pierre-Doray
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov added 1 comment![]( "Open in Gerrit")
std::vector<TraceLog::TrackEventSession> TraceLog::GetTrackEventSessions()Haha, I forgot to implement it when I added this function 😄 Thanks for fixing!
Eric Seckler (Gerrit)
Eric Seckler voted and added 3 comments![]( "Open in Gerrit")
Votes added by Eric Seckler
| Code-Review | +1 |
3 comments
- Some reordering of operations in early startup to ensure dependancies"dependancies" is a possible misspelling of "dependencies".
nit :)
TRACING_EXPORT extern const char kTraceStartupRecordMode[];Etienne Pierre-DorayWondering if there are any real users of this left now.
The equivalent of this is used in config file (in telemetry), so maybe it makes sense to keep.
Hm. Telemetry doesn't use circular buffers AFAIK -- and alerts when data from traces is missing due to buffer exhaustion etc. But we don't have to remove this right away; can do this separately.
perfetto::DataSourceConfig data_source_config =
trace_log->GetCurrentTrackEventDataSourceConfig();
if (data_source_config.has_interceptor_config()) {
return base::ReadOnlySharedMemoryRegion();
}
*trace_config.add_data_sources()->mutable_config() = data_source_config;- Etienne Pierre-Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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
base::ReadOnlySharedMemoryRegion();return
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
- Some reordering of operations in early startup to ensure dependancies"dependancies" is a possible misspelling of "dependencies".
nit :)
Done
base::ReadOnlySharedMemoryRegion();Etienne Pierre-Dorayreturn
Done Good catch!
perfetto::DataSourceConfig data_source_config =
trace_log->GetCurrentTrackEventDataSourceConfig();
if (data_source_config.has_interceptor_config()) {
return base::ReadOnlySharedMemoryRegion();
}
*trace_config.add_data_sources()->mutable_config() = data_source_config;- Dave Tapuska
Code-Coverage
Code-Review
| 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")
OverrideFrameworkBundlePath();Why are these still in content/shell/app/shell_main_delegate.cc then?
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
Why are these still in content/shell/app/shell_main_delegate.cc then?
For the browser process. I think moving these to content/public/test/browser_test_base.cc should work too; let's see what CQ says.
Note: moving them to content/public/test/test_launcher.cc doesn't work since the code is from a different bundle.
Related details
- Dave Tapuska
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
| 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
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| 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")
I think this looks ok. But does feel odd that we are putting these in content/public/app now since these are only used in content shell derivates. So I'm tag-teaming in Avi (as a content owner) who might have some ideas around the bundle ids..
Related details
- Avi Drissman
- Eric Seckler
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
This strikes me as not ideal. This fails the Content API rules as being an implementation and not an interface, but this fails also because the implementation in the source file is descriptive of how Content Shell works, not content/ in general.
I’m not up to speed on the core problem that we’re trying to solve here by moving this up a level from Shell.
Related details
- Eric Seckler
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
This strikes me as not ideal. This fails the Content API rules as being an implementation and not an interface, but this fails also because the implementation in the source file is descriptive of how Content Shell works, not content/ in general.
I’m not up to speed on the core problem that we’re trying to solve here by moving this up a level from Shell.
Nevermind, this didn't work so I'm reverting to (almost) patchset 40.
I'll keep the override calls in shell_main_delegate.cc for the browser process.
Related details
- Avi Drissman
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| 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")
Etienne Pierre-DorayThis strikes me as not ideal. This fails the Content API rules as being an implementation and not an interface, but this fails also because the implementation in the source file is descriptive of how Content Shell works, not content/ in general.
I’m not up to speed on the core problem that we’re trying to solve here by moving this up a level from Shell.
Nevermind, this didn't work so I'm reverting to (almost) patchset 40.
I'll keep the override calls in shell_main_delegate.cc for the browser process.
So I suspected you moved them into shell_content_main.cc because you needed them earlier. Is that correct?
Leaving them in shell_main_delegate.cc isn't ideal because won't they get executed twice?
What was the error with removing them from shell_main_delegate.cc?
I would have thought you would have removed them there but added them to https://source.chromium.org/chromium/chromium/src/+/main:content/test/content_test_launcher.cc;l=93?q=content%2Fshell&ss=chromium%2Fchromium%2Fsrc:content%2Ftest%2F so that covers the other case where ContentShellMainDelegate is instantiated.
Related details
- Avi Drissman
- Eric Seckler
- Etienne Pierre-Doray
- Mikhail Khokhlov
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Etienne Pierre-DorayThis strikes me as not ideal. This fails the Content API rules as being an implementation and not an interface, but this fails also because the implementation in the source file is descriptive of how Content Shell works, not content/ in general.
I’m not up to speed on the core problem that we’re trying to solve here by moving this up a level from Shell.
Dave TapuskaNevermind, this didn't work so I'm reverting to (almost) patchset 40.
I'll keep the override calls in shell_main_delegate.cc for the browser process.
So I suspected you moved them into shell_content_main.cc because you needed them earlier. Is that correct?
Leaving them in shell_main_delegate.cc isn't ideal because won't they get executed twice?
What was the error with removing them from shell_main_delegate.cc?
I would have thought you would have removed them there but added them to https://source.chromium.org/chromium/chromium/src/+/main:content/test/content_test_launcher.cc;l=93?q=content%2Fshell&ss=chromium%2Fchromium%2Fsrc:content%2Ftest%2F so that covers the other case where ContentShellMainDelegate is instantiated.
Yes, we'd call overrides twice (though second time has no impact).
I tried moving the calls to content/test/content_test_launcher.cc and I'm getting:
```
[0626/150514.012292:FATAL:paths_mac.mm(41)] Check failed: "Contents" == path.BaseName().value() (Contents vs. out)
0 libbase.dylib 0x000000014d422450 base::debug::CollectStackTrace(void const**, unsigned long) + 48
1 libbase.dylib 0x000000014d3c88ec base::debug::StackTrace::StackTrace(unsigned long) + 112
2 libbase.dylib 0x000000014d3c8994 base::debug::StackTrace::StackTrace(unsigned long) + 36
3 libbase.dylib 0x000000014d3c8960 base::debug::StackTrace::StackTrace() + 40
4 libbase.dylib 0x000000014d06d344 logging::LogMessage::Flush() + 216
5 libbase.dylib 0x000000014d06d250 logging::LogMessage::~LogMessage() + 44
6 libbase.dylib 0x000000014d002a4c logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 152
7 libbase.dylib 0x000000014d002974 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 28
8 libbase.dylib 0x000000014d0029a0 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 28
9 libbase.dylib 0x000000014d002f94 std::__Cr::default_delete<logging::LogMessage>::operator()(logging::LogMessage*) const + 52
10 libbase.dylib 0x000000014d001234 std::__Cr::unique_ptr<logging::LogMessage, std::__Cr::default_delete<logging::LogMessage>>::reset(logging::LogMessage*) + 104
11 libbase.dylib 0x000000014d001184 logging::CheckError::~CheckError() + 68
12 libbase.dylib 0x000000014d00128c logging::CheckError::~CheckError() + 28
13 content_browsertests 0x000000010b6905f4 (anonymous namespace)::GetContentsPath() + 900
14 content_browsertests 0x000000010b690118 (anonymous namespace)::GetFrameworksPath() + 44
15 content_browsertests 0x000000010b690040 OverrideFrameworkBundlePath() + 60
16 content_browsertests 0x00000001077f7954 main + 100
17 dyld 0x000000019b77a0e0 start + 2360
```
This is because content/test/content_test_launcher.cc is in out/Debug/content_browsertests
whereas content/shell/app/shell_main_delegate_mac.mm is in
out/Debug/Content Shell.app/Contents/MacOS/Content Shell (content_shell_framework is a mac_framework_bundle target).
Related details
- Avi Drissman
- Dave Tapuska
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Also, removing the overrides from shell_main_delegate.cc gives us the default bundle id ('org.chromium.Chromium'), which fails tests that need rendezvous.
I get the same error as https://chromium-swarm.appspot.com/task?id=6a238df8ba527111&o=true&w=true
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
Did you try moving these into the ctor from ShellMainDelegate::BasicStartupComplete?
Related details
- Avi Drissman
- Eric Seckler
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
I just tried, I'm getting the same error (the constructor runs content_browsertests). I'm not sure why; maybe it gets inlined (BasicStartupComplete is exposed through virtual ContentMainDelegate)..
Related details
- Avi Drissman
- Dave Tapuska
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| 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")
I think I've got it (for real this time) by moving override calls to ContentBrowserTest.
Related details
- Avi Drissman
- Dave Tapuska
- Eric Seckler
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler voted Code-Review+1![]( "Open in Gerrit")
- Avi Drissman
- Dave Tapuska
- Etienne Pierre-Doray
- Mikhail Khokhlov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Avi Drissman
- Dave Tapuska
- Etienne Pierre-Doray
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Avi Drissman
- Etienne Pierre-Doray
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Avi Drissman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[tracing] Forward startup tracing config as shmem
Using base::shared_memory::AddToLaunchParameters() to
send config in a shmem at child process creation.
Related changes:
- Some reordering of operations in early startup to ensure dependencies
are in place.
- TraceStartupConfig holds a perfetto::TraceConfig instead of
a base::TraceConfig
- M base/trace_event/trace_log.cc
- M chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc
- M components/tracing/common/tracing_switches.cc
- M components/tracing/common/tracing_switches.h
- M content/app/content_main_runner_impl.cc
- M content/browser/browser_child_process_host_impl.cc
- M content/browser/browser_child_process_host_impl.h
- M content/browser/child_process_launcher.cc
- M content/browser/child_process_launcher.h
- M content/browser/child_process_launcher_helper.cc
- M content/browser/child_process_launcher_helper.h
- M content/browser/devtools/protocol/tracing_handler.cc
- M content/browser/gpu/gpu_process_host.cc
- M content/browser/ppapi_plugin_process_host.cc
- M content/browser/renderer_host/render_process_host_impl.cc
- M content/browser/tracing/background_startup_tracing_observer_unittest.cc
- M content/browser/tracing/background_tracing_config_impl.cc
- M content/browser/tracing/background_tracing_config_impl.h
- M content/browser/tracing/background_tracing_manager_browsertest.cc
- M content/browser/tracing/startup_tracing_browsertest.cc
- M content/browser/tracing/startup_tracing_controller.cc
- M content/browser/utility_process_host.cc
- M content/public/common/content_descriptors.h
- M content/public/test/content_browser_test.cc
- M content/shell/app/paths_mac.h
- M content/shell/app/paths_mac.mm
- M content/shell/app/shell_content_main.cc
- M content/shell/app/shell_main_delegate.cc
- M content/shell/app/shell_main_delegate_mac.h
- M content/shell/app/shell_main_delegate_mac.mm
- M content/test/content_browser_test_test.cc
- M services/tracing/public/cpp/trace_startup.cc
- M services/tracing/public/cpp/trace_startup.h
- M services/tracing/public/cpp/trace_startup_config.cc
- M services/tracing/public/cpp/trace_startup_config.h
- M services/tracing/public/cpp/trace_startup_config_unittest.cc
Code-Review: +1 by Dave Tapuska, +1 by Mikhail Khokhlov, +1 by Eric Seckler
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |