Skip to content

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

Merged
merged 3 commits into from
Jun 18, 2025

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented Jun 18, 2025

This updates the expected data to match the specification.

@armenzg armenzg self-assigned this Jun 18, 2025
@armenzg armenzg requested review from a team as code owners June 18, 2025 14:54
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 18, 2025
report_type = report.get("type")
metrics.incr(
"browser_reporting.raw_report_received",
tags={"browser_report_type": report_type},
Copy link
Member Author

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.

Copy link

codecov bot commented Jun 18, 2025

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ry/issues/endpoints/browser_reporting_collector.py 90.00% 2 Missing ⚠️
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           

@armenzg armenzg merged commit 67e630b into master Jun 18, 2025
64 checks passed
@armenzg armenzg deleted the fix/browser_report/multiple_reports/armenzg branch June 18, 2025 16:46
Copy link
Member

@lobsterkatie lobsterkatie left a 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.

Comment on lines +35 to +43
@dataclass
class BrowserReport:
body: dict[str, Any]
type: BrowserReportType
url: str
user_agent: str
destination: str
timestamp: int
attempts: int
Copy link
Member

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},
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

The typing protects us.

Copy link
Member

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?

Copy link
Member Author

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 :)

Comment on lines +18 to +32
{
"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,
}
Copy link
Member

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.

andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants