-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(browser-report): Adjust code to specification #93818
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
Conversation
This updates the expected data to match [the specification](https://www.w3.org/TR/reporting/#concept-reports).
report_type = report.get("type") | ||
metrics.incr( | ||
"browser_reporting.raw_report_received", | ||
tags={"browser_report_type": report_type}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also changing the tag.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93818 +/- ##
=======================================
Coverage 88.02% 88.02%
=======================================
Files 10331 10331
Lines 596425 596453 +28
Branches 23160 23160
=======================================
+ Hits 524995 525028 +33
+ Misses 70974 70969 -5
Partials 456 456 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job remembering it's a list. Definitely forgot to account for that. See below about the individual fields.
@dataclass | ||
class BrowserReport: | ||
body: dict[str, Any] | ||
type: BrowserReportType | ||
url: str | ||
user_agent: str | ||
destination: str | ||
timestamp: int | ||
attempts: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you're looking at the wrong source for your list of fields. These are the fields in the report object generated by the browser, but not the ones in the serialized report that's sent. See also the sample reports in the spec (which match what my test app sends).
browser_report = BrowserReport(**report) | ||
metrics.incr( | ||
"browser_reporting.raw_report_received", | ||
tags={"browser_report_type": browser_report.type}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have the same issue here as before? Of a rogue actor potentially sending arbitrary values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typing protects us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types aren't checked at runtime, though, are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pff you got me there :)
{ | ||
"body": { | ||
"columnNumber": 12, | ||
"id": "RangeExpand", | ||
"lineNumber": 31, | ||
"message": "Range.expand() is deprecated. Please use Selection.modify() instead.", | ||
"sourceFile": "https://dogs.are.great/_next/static/chunks/_4667019e._.js", | ||
}, | ||
"type": "deprecation", | ||
"url": "https://dogs.are.great/", | ||
"user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/137.0.0.0 Safari/537.36", | ||
"destination": "default", | ||
"timestamp": 1640995200000, # January 1, 2022 in milliseconds | ||
"attempts": 1, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above re: the list of fields here.
This updates the expected data to match [the specification](https://www.w3.org/TR/reporting/#concept-reports).
This updates the expected data to match the specification.