Skip to content
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

Initial implementation of parseCrashes() #1

Merged
merged 8 commits into from
Oct 20, 2022

Conversation

arturocuya
Copy link
Contributor

@arturocuya arturocuya commented Mar 11, 2022

The base tool structures only the paths found in the crash report. The goal of this PR is to also structure the remaining information for each crash:

  • The error message
  • The hardware platforms where the crash occured and their count. Not only the code name but also additional information (taken from here) like the product name and the model, e.g: Roku Ultra, model 4800X.
  • The application versions and the error count for each.
  • The stacktrace, with the scope names and locations of each scope level
  • The local variable names and their metadata

This structured list of crashes is stored in CrashlogFile.crashes. Now CrashlogFile.crashes and CrashlogFile.references have redundant information, but I have not removed the latter because I don't know much about the codebase outside of CrashlogFile.

I have left some TODOs with questions. Please address them adding comments to the PR.

src/CrashlogFile.spec.ts Outdated Show resolved Hide resolved
src/CrashlogFile.spec.ts Outdated Show resolved Hide resolved
src/CrashlogFile.ts Outdated Show resolved Hide resolved
src/CrashlogFile.ts Outdated Show resolved Hide resolved
src/CrashlogFile.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
@arturocuya
Copy link
Contributor Author

@TwitchBronBron What about the redundant information in CrashlogFile.crashes and CrashlogFile.references?

src/CrashlogFile.ts Outdated Show resolved Hide resolved
src/interfaces.ts Outdated Show resolved Hide resolved
@TwitchBronBron
Copy link
Member

@TwitchBronBron What about the redundant information in CrashlogFile.crashes and CrashlogFile.references?

There might be situations where there are pkg paths not found inside a crash. The references collection handles every pkgPath in the entire crashlog.

Perhaps for CrashlogFile.crashes, you could add a property called location that points to the specific CrashlogFile.references entry. So instead of crash.pkgPath, you would do crash.location.pkgPath. That way, you get the sourcemap lookups for free since that location would be a reference to the existing CrashlogFile.references item.

@arturocuya
Copy link
Contributor Author

@TwitchBronBron Update:

  • Improved stack trace parsing and added tests for it.
  • Removed unnecesary loop.
  • Turned off linter rule @typescript-eslint/no-loop-func
  • Renamed StackTraceStep to StackFrame
  • Replaced StackFrame's pkgLocation and srcLocation for reference which is a pointer to a CrashlogFile.references entry.


const match = /(\w+:\/.*?)\((\d+)\)/.exec(pkgLocationAsArray.join(' ').trim());
if (match) {
// We need the range to calculate the reference index. To generate the range, we need the line number.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TwitchBronBron I honestly couldn't think of a more elegant solution for this. I wanted to make the subparsers for each crash section agnostic to the order of the sections, so Roku can miss sending, for example, the hardware information and the parser could still figure out the stack trace. This resulted in the subparser for the stack trace not having the original line number for each of its section lines. Hence this code...

Copy link
Member

Choose a reason for hiding this comment

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

If it's accurate, then I'm fine with this for now. We can improve this over time.

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Everything looks good here. Is there anything else to do before I merge this? The only thing I can see is to get the CI failures fixed.

@arturocuya
Copy link
Contributor Author

Everything looks good here. Is there anything else to do before I merge this? The only thing I can see is to get the CI failures fixed.

@TwitchBronBron Yep, just wait for me to fix the coverage problem.

@TwitchBronBron TwitchBronBron merged commit e38c1d8 into rokucommunity:master Oct 20, 2022
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