[netlog] Conditionally support full netlog data in perfetto traces [chromium/src : main]
Camillo Bruni (Gerrit)
Camillo Bruni added 1 comment![]( "Open in Gerrit")
@edechamps PTAL components/cronet
@petrcermak PTAL base/trace_event
@mmenke PTAL the rest
Related details
- Etienne Dechamps
- 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 1 comment![]( "Open in Gerrit")
Have you verified the trace viewer can actually handle logs with everything?
Related details
- Camillo Bruni
- Etienne Dechamps
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 2 comments![]( "Open in Gerrit")
// - Explicitly provided value will take precedence.
// - If command line --net-log-mode is available we use that.This comment is confusing. It's what NetworkService::NetworkService does when creating a NetLogObserver, not what this class does when passed a null `capture_mode`.
This is attached to a bug that's apparently targetting Cronet, but Cronet doesn't use NetworkService, or Mojo at all. Why are we making changes to NetworkService?
net::NetLogCaptureMode GetNetCaptureModeFromCommandLine(I'm not comfortable with command line flags affecting TraceNetLog observer (i.e., anything other than --log-net-log=). Can we do something more trace-API-friendly? e.g., use a different trace category.
TraceNetLogObserver::SelectCaptureMode() already seems to do this, in fact, so I'm not clear on why this is needed.
Related details
- Camillo Bruni
- Etienne Dechamps
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. |
Camillo Bruni (Gerrit)
Camillo Bruni added 3 comments![]( "Open in Gerrit")
Have you verified the trace viewer can actually handle logs with everything?
Yes, I have a WIP network panel for perfetto that displays most of the data that is now captured (so far I've tried with traces created on linux).
https://github.com/google/perfetto/compare/dev/cbruni/2026-05-22_network_panel
// - Explicitly provided value will take precedence.
// - If command line --net-log-mode is available we use that.This comment is confusing. It's what NetworkService::NetworkService does when creating a NetLogObserver, not what this class does when passed a null `capture_mode`.
This is attached to a bug that's apparently targetting Cronet, but Cronet doesn't use NetworkService, or Mojo at all. Why are we making changes to NetworkService?
- oops, this should be `--net-log-capture-mode`
Re why the changes:
- currently we can't get full netlogs in perfetto
- cronet uses TraceNetLogObserver with a slightly custom trace_net_log_capture_mode approach that I do not want to change since I'm not sure about the implications (maybe components/cronet/android/cronet_library_loader.cc is unused?)
net::NetLogCaptureMode GetNetCaptureModeFromCommandLine(I'm not comfortable with command line flags affecting TraceNetLog observer (i.e., anything other than --log-net-log=). Can we do something more trace-API-friendly? e.g., use a different trace category.
TraceNetLogObserver::SelectCaptureMode() already seems to do this, in fact, so I'm not clear on why this is needed.
IMO it's more consistent with using the other netlog observers but no strong opinon on this (it was mentioned in older comments that this might be pending work see the cronet todo for https://crbug.com/410018349).
I personally have the preference of command line flags having higher priority than trace configs. And thus I was a bit confused initially when `--net-log-capture-mode=...` had no impact on the trace data.
I can revert this part since the crucial bit is getting the data to perfetto.
Related details
- 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. |
Etienne Dechamps (Gerrit)
Etienne Dechamps added 1 comment![]( "Open in Gerrit")
// - Explicitly provided value will take precedence.
// - If command line --net-log-mode is available we use that.Camillo BruniThis comment is confusing. It's what NetworkService::NetworkService does when creating a NetLogObserver, not what this class does when passed a null `capture_mode`.
This is attached to a bug that's apparently targetting Cronet, but Cronet doesn't use NetworkService, or Mojo at all. Why are we making changes to NetworkService?
- oops, this should be `--net-log-capture-mode`
Re why the changes:
- currently we can't get full netlogs in perfetto
- cronet uses TraceNetLogObserver with a slightly custom trace_net_log_capture_mode approach that I do not want to change since I'm not sure about the implications (maybe components/cronet/android/cronet_library_loader.cc is unused?)
It is definitely used. See https://chromium.googlesource.com/chromium/src/+/main/components/cronet/tracing.md for how Cronet tracing works. If you are making any changes to how this works, please update tracing.md accordingly in this CL.
Related details
- Camillo Bruni
- 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. |
Camillo Bruni (Gerrit)
Camillo Bruni added 1 comment![]( "Open in Gerrit")
// - Explicitly provided value will take precedence.
// - If command line --net-log-mode is available we use that.Camillo BruniThis comment is confusing. It's what NetworkService::NetworkService does when creating a NetLogObserver, not what this class does when passed a null `capture_mode`.
This is attached to a bug that's apparently targetting Cronet, but Cronet doesn't use NetworkService, or Mojo at all. Why are we making changes to NetworkService?
Etienne Dechamps
- oops, this should be `--net-log-capture-mode`
Re why the changes:
- currently we can't get full netlogs in perfetto
- cronet uses TraceNetLogObserver with a slightly custom trace_net_log_capture_mode approach that I do not want to change since I'm not sure about the implications (maybe components/cronet/android/cronet_library_loader.cc is unused?)
It is definitely used. See https://chromium.googlesource.com/chromium/src/+/main/components/cronet/tracing.md for how Cronet tracing works. If you are making any changes to how this works, please update tracing.md accordingly in this CL.
ok, then this makes sense - and I've tried to be behavior preserving for cronet.
Related details
- Etienne Dechamps
- 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. |
Matt Menke (Gerrit)
Matt Menke added 1 comment![]( "Open in Gerrit")
net::NetLogCaptureMode GetNetCaptureModeFromCommandLine(Camillo BruniI'm not comfortable with command line flags affecting TraceNetLog observer (i.e., anything other than --log-net-log=). Can we do something more trace-API-friendly? e.g., use a different trace category.
TraceNetLogObserver::SelectCaptureMode() already seems to do this, in fact, so I'm not clear on why this is needed.
IMO it's more consistent with using the other netlog observers but no strong opinon on this (it was mentioned in older comments that this might be pending work see the cronet todo for https://crbug.com/410018349).
I personally have the preference of command line flags having higher priority than trace configs. And thus I was a bit confused initially when `--net-log-capture-mode=...` had no impact on the trace data.
I can revert this part since the crucial bit is getting the data to perfetto.
This CL introduces trace categories for this. I don't think we should have a way to override them. Command line doesn't affect about:net-export either, because it has its own, user-visible way to set mode. Given the sensitivity of NetLog data, there should be as few surprises as possible (admittedly, NetLog being global and not per-profile is a surprise as well, unfortunately)
Related details
- Camillo Bruni
- Etienne Dechamps
- 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 added 5 comments![]( "Open in Gerrit")
NetLogCaptureMode TraceNetLogObserver::SelectCaptureMode() const {Definition order should match order in header file.
NetLogCaptureMode TraceNetLogObserver::SelectCaptureMode() const {Let's call this GetCaptureMode() or CaptureMode(). Select implies to me, at least, that it sets state (i.e., `capture_mode_ = foo`)
if (net_log() || !net_log_to_watch_) {Can this happen? The only place this is cleared is StopWatchForTraceStart(), and this shouldn't be called after we stop watching for trace start, no? Only place it's set is WatchForTraceStart(), which doesn't seem to support it being nullptr.
void TriggerByteTransferAndFlush(
std::optional<std::string_view> category,Should mention what an empty category means (we don't actually enable tracing). SHould also mention the byte transfer event.
const base::Value* item = &trace_events()[0];
ASSERT_TRUE(item->is_dict());Can we check that this doesn't have args.params.bytes? Same for the others - and we should make sure we have something that wouldn't appear in non-sensitive logs as well. Pulling the actual category should ideally ensure all that works, anyways, but as long as it's not that difficult, seems safest to be complete here.
We are already relying on the details of byte transfer event logic.
Related details
- Camillo Bruni
- Etienne Dechamps
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. |