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

added setExtraParameters to crashReporter #7072

Closed
wants to merge 2 commits into from

Conversation

TimvdEijnden
Copy link

We want to send along the logged in user in our application when submitting a crash report.
But we want to start the crash report directly on start so we wont miss any crashes.

When we load our website we can get the logged in user from the loaded website and add that info to the extra_parameters of the crashReporter. This was not possible so i've added it.

screen shot 2016-09-02 at 17 08 35

### `crashReporter.setExtraParameters(extra)`

* `extra` Object - An object you can define that will be sent along with the
report. Only string properties are sent correctly, Nested objects are not
Copy link
Contributor

Choose a reason for hiding this comment

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

correctly, Nested -> correctly. Nested

Copy link
Author

Choose a reason for hiding this comment

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

I've copied it from the start method docs 😉

@zeke
Copy link
Contributor

zeke commented Sep 2, 2016

Nice, @TimvdEijnden. Are you able to add some tests for this new feature?

@zeke
Copy link
Contributor

zeke commented Sep 2, 2016

Seeing a failure on Windows CI. It seems unrelated though. Maybe a flaky build?

[1228/1228] LINK_EMBED electron.exe
Running test.py
Attempted to load extension "foo" that has already been loaded.
Reading C:\projects\electron\spec\does-not-exist\manifest.json failed.
Error: ENOENT: no such file or directory, open 'C:\projects\electron\spec\does-not-exist\manifest.json'
    at Error (native)
    at Object.fs.openSync (fs.js:640:18)
    at Object.module.(anonymous function) [as openSync] (ELECTRON_ASAR.js:167:20)
    at Object.fs.readFileSync (fs.js:508:33)
    at Object.fs.readFileSync (ELECTRON_ASAR.js:500:29)
    at getManifestFromPath (C:\projects\electron\out\D\resources\electron.asar\browser\chrome-extension.js:33:26)
    at Function.BrowserWindow.addDevToolsExtension (C:\projects\electron\out\D\resources\electron.asar\browser\chrome-extension.js:363:22)
    at callFunction (C:\projects\electron\out\D\resources\electron.asar\browser\rpc-server.js:217:18)
    at EventEmitter.<anonymous> (C:\projects\electron\out\D\resources\electron.asar\browser\rpc-server.js:306:5)
    at emitMany (events.js:127:13)
Parsing C:\projects\electron\spec\fixtures\devtools-extensions\bad-manifest\manifest.json failed.
SyntaxError: Unexpected token } in JSON at position 0
    at Object.parse (native)
    at getManifestFromPath (C:\projects\electron\out\D\resources\electron.asar\browser\chrome-extension.js:41:21)
    at Function.BrowserWindow.addDevToolsExtension (C:\projects\electron\out\D\resources\electron.asar\browser\chrome-extension.js:363:22)
    at callFunction (C:\projects\electron\out\D\resources\electron.asar\browser\rpc-server.js:217:18)
    at EventEmitter.<anonymous> (C:\projects\electron\out\D\resources\electron.asar\browser\rpc-server.js:306:5)
    at emitMany (events.js:127:13)
    at EventEmitter.emit (events.js:201:7)
    at WebContents.<anonymous> (C:\projects\electron\out\D\resources\electron.asar\browser\api\web-contents.js:218:13)
    at emitTwo (events.js:106:13)

@TimvdEijnden
Copy link
Author

I'll try to add tests, I have zero knowledge of C++ 😇

@TimvdEijnden
Copy link
Author

TimvdEijnden commented Sep 5, 2016

I was writing tests to verify if my changes worked, but it doesn't work :(

The implementation is different per OS, I think it's possible to add this support but I don't have the experience to fix this. It's too difficult for me.

@@ -58,6 +58,17 @@ sent or the crash reporter has not been started, `null` is returned.
Returns all uploaded crash reports. Each report contains the date and uploaded
ID.

### `crashReporter.setExtraParameters(extra)`
Copy link
Contributor

Choose a reason for hiding this comment

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

So these are meant to completely replace the extra parameters specified to start right? Maybe the docs should mention something like This will override any extra parameters specified to start() just to make things clear.

@bridiver
Copy link
Contributor

bridiver commented Sep 8, 2016

I've been working on pulling in support base::debug::SetCrashKeyValue and will have a PR ready soon. Seems like we could combine the two and add an electron api for setCrashKeyValue that would add the values to the crash metadata just like they do in chrome

@TimvdEijnden
Copy link
Author

Yes that would be great. Adding the setExtraParameters was easy but getting it to work is to hard.
As long as I can add extra data after starting the crash reporter I'm happy.

I'm closing this for now and will wait for your PR.

@zeke
Copy link
Contributor

zeke commented Oct 5, 2016

I've been working on pulling in support base::debug::SetCrashKeyValue and will have a PR ready soon.

Hey @bridiver have you made any progress on this? If not, I can pick it up.

@bridiver
Copy link
Contributor

@zeke I have a partially complete branch. I think I should be able to pick it back up and finish, but if not I'll link you to what I have and you can pick it up from there

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.

None yet

4 participants