Skip to content

core(network-requests): add initiators to debugData #16605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

bolu-tife
Copy link
Contributor

Summary

This is a feature request for #11123 to identify a network request initiator from the network-requests audit log

@bolu-tife bolu-tife requested a review from a team as a code owner July 24, 2025 15:27
@bolu-tife bolu-tife requested review from connorjclark and removed request for a team July 24, 2025 15:27
@bolu-tife
Copy link
Contributor Author

Follow up from #16599

Thanks for taking this on!

Sorry we didn't respond to your earlier questions in #11123. I think this is still useful, but I'll request a small change:

initiator: {type, url, lineNumber, columnNumber}

I think the type may be useful, and I prefer to keep the data here structured rather than flat. Also might as well include the column if we're showing the line number.

Only thing we're leaving out at that point is the stack (too much info imo) and the requestId (the url is sufficient).

@bolu-tife
Copy link
Contributor Author

bolu-tife commented Jul 24, 2025

I noticed that tableDetails.items (LH.Audit.Details.Table['items']) doesn't support NetworkRequest[], so I added the initiator data to debugData, which supports it because it allows a type any.
I also filtered by initiator.url because not all initiators have a URL, and even those that do might not include a lineNumber or columnNumber. This also filters out initiators with structure {"type": "other"}.

What do you think about this approach? Should it be handled differently, such as by amending the existing types? Or should the initiators be left as-is to maintain alignment with audit.details.items, even when some have no url or have undefined values for lineNumber and columnNumber?

@bolu-tife
Copy link
Contributor Author

I'd update the failing test

@bolu-tife bolu-tife marked this pull request as draft July 24, 2025 15:37
@bolu-tife bolu-tife marked this pull request as ready for review July 24, 2025 20:45
type,
...(url && {url: UrlUtils.elideDataURI(url)}),
...(lineNumber && {lineNumber}),
...(columnNumber && {columnNumber}),
Copy link
Collaborator

@connorjclark connorjclark Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line/col are zero based, so this would not include the properties when 0.

I think just including the properties as-is ({type, url: url ? UrlUtils.elideDataURI(url) : undefined, lineNumber, columnNumber}) should be fine (for example). If they are undefined then they get dropped when we serialize to JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. I'd make the changes shortly.

@connorjclark
Copy link
Collaborator

connorjclark commented Jul 29, 2025

Good point about the details type not supporting arbitrary object shapes. Forgot about that :) debugData was a good idea!

What do you think about this approach? Should it be handled differently, such as by amending the existing types? Or should the initiators be left as-is to maintain alignment with audit.details.items, even when some have no url or have undefined values for lineNumber and columnNumber?

it should be fine to include always in the debugData. When serialized to JSON, if they were undefined they will get dropped.

@connorjclark
Copy link
Collaborator

FYI you can ignore the failing smoke test, it's unrelated.

@connorjclark connorjclark changed the title core(network-requests): add network initiator data core(network-requests): add initiators to debugData Jul 31, 2025
@connorjclark
Copy link
Collaborator

Thanks!

@connorjclark connorjclark merged commit 6e08d32 into GoogleChrome:main Jul 31, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants