-
Notifications
You must be signed in to change notification settings - Fork 94
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
Bug 1631506 - Add some more fields from crash annotations #546
Conversation
Integration report for "Bug 1631506 - Add some more fields from crash annotations"Report for upstream
|
c77cdd6
to
2c2d351
Compare
I wouldn't mind getting @acmiyaguchi to look at this one, since he's handled this type of case before. Would welcome @relud's opinion on some of the overly-long description fields in this schema though -- not sure how those will translate to e.g. bigquery's schema browser (some of them have e.g. newlines) |
Integration report for "Bug 1631506 - Add some more fields from crash annotations"Report for upstream
|
looks like the console's schema browser just renders any group of whitespace as a single space: # bq show --schema relud-17123:test.test_desc | jq .
[
{
"type": "STRING",
"name": "field1",
"description": "<levels>, Optional, present only if >0,\n\nstuff in another couple lines\nthat spans it's own two lines."
}
] |
@@ -1296,18 +1300,34 @@ | |||
"description": "<time>, Seconds since the Epoch (see also: `payload.crashTime`)", | |||
"type": "string" | |||
}, | |||
"DOMFissionEnabled": { | |||
"description": "Set to 1 when DOM fission is enabled, and subframes are potentially loaded in a separate process.", | |||
"type": "boolean" |
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 have reason to believe this is not the correct type, and that all types within the metadata field should be a string. Let me put together a query to confirm this.
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.
All of the types reported by payload.metadata
should be a string, as per this query: https://sql.telemetry.mozilla.org/queries/71019/source. If you extract the value on DOMFissionEnabled
, the values within are actually integers (1/0), and not booleans (true/false), so this would cause an increase in validation errors.
Adding the original type to the description would be helpful as a prefix e.g.[{type}]: {description}
.
Integration report for "Update scripts/extract_crash_annotation_fields"Report for upstream
|
Also add a small script, extract_crash_annotation_fields for automatically grabbing this type of information in the future.
Co-authored-by: Anthony Miyaguchi <acmiyaguchi@gmail.com>
f2e601b
to
aa03913
Compare
Thanks @acmiyaguchi! Great suggestions and good catch on the types. I've updated both the generation script and the schemas per your suggestions |
@@ -1272,6 +1272,10 @@ | |||
"description": "<size>, Windows-only, available physical memory in bytes", | |||
"type": "string" | |||
}, | |||
"AvailableSwapMemory": { | |||
"description": "<size>, Amount of free swap space in bytes. - Under macOS, populated with the contents of\n sysctl \"vm.swapusage\" :: xsu_avail.\n- Under Linux, populated with /proc/meminfo's SwapFree. - Not available on other platforms.", |
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 modified these from the output of my script to correspond with other fields of similar kind in the file.
Integration report for "Regenerate schemas"Report for upstream
|
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.
Looks good to me.
0e9517a 2020-05-12 15:37:16 -0700 Bug 1636317 - Add metadata schema for Pioneer ingestion (#544) 2832433 2020-05-12 13:29:06 -0700 Update README.pioneer.md (#548) 11d3fa2 2020-05-12 14:59:44 -0400 Optimize Dockerfile 22ce6d8 2020-05-12 14:31:31 -0400 Bug 1631506 - Add some more fields from crash annotations (#546)
Also add a small script, extract_crash_annotation_fields for
automatically grabbing this type of information in the future.
Checklist for reviewer:
.circleci/config.yml
) will cause environment variables (particularly credentials) to be exposed in test logsintegration
CI test by pushing this revision as discussed in the README and review the report posted in the comments.For glean changes:
include/glean/CHANGELOG.md