feat(telemetry): Crash Monitoring #5684
Conversation
|
This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions. |
nkomonen-amazon
changed the title
nkomonen-amazon
force-pushed
the
crashReporting
branch
6 times, most recently
from
9b5f475 to
72bd7f5
Compare
nkomonen-amazon
changed the title
| }, | ||
| forceIdeCrash: { | ||
| label: 'Crash: Force IDE ExtHost Crash', | ||
| detail: `Will SIGKILL ExtHost, { pid: ${process.pid}, sessionId: '${getSessionId().slice(0, 8)}-...' }, but the IDE itself will not crash.`, |
There was a problem hiding this comment.
is this different than vscode's Restart Extension Host command?
There was a problem hiding this comment.
Restart Extension Host still results in a graceful shutdown of the extension host (deactivate() runs), while this one is non-graceful
There was a problem hiding this comment.
does this need its own subdir or could it live in shared/crashmonitoring.ts
There was a problem hiding this comment.
Yeah in its current state this makes sense. Will update
| /** Start the Crash Monitoring process */ | ||
| public async start() { | ||
| if (isWeb()) { | ||
| return |
There was a problem hiding this comment.
maybe worth a log message
| return | |
| debug('skipping crash monitoring in web-mode') | |
| return |
| } | ||
|
|
||
| // This is the metric to let us know the extension crashed | ||
| telemetry.session_end.emit({ |
There was a problem hiding this comment.
can we force the proxied sessionId into the sessionId field (and eliminate proxiedSessionId)? there's not really any use for the sessionId field from the current (non-proxied) session.
There was a problem hiding this comment.
sessionId is adjacent to clientId and is injected in a different part of the metric right before sending:
We have to do some hacky stuff to get it to work but it is possible, though I don't think is worth the effort atm
justinmk3
left a comment
justinmk3
left a comment
There was a problem hiding this comment.
can we remove getPids, binarySearch, and dependencies proper-lock , ps-list in this PR
@justinmk3 opened PR here: #5707 |
nkomonen-amazon
force-pushed
the
crashReporting
branch
5 times, most recently
from
b3e148c to
38ffb89
Compare
|
/runIntegrationTests |
nkomonen-amazon
force-pushed
the
crashReporting
branch
from
38ffb89 to
1d8698e
Compare
packages/core/src/shared/errors.ts
Outdated
| 'globalStorage', // from vscode globalStorageUri | ||
| CrashMonitoring.rootDir, | ||
| CrashMonitoring.runningExtDir, | ||
| CrashMonitoring.shutdownExtDir, |
There was a problem hiding this comment.
is this going to un-scrub the user's entire /tmp dir path, or only the last segment?
There was a problem hiding this comment.
These are just the dir names, not the full path. As part of the move to the main constants file I've renamed these variables to be more clear.
nkomonen-amazon
force-pushed
the
crashReporting
branch
from
5ca81d0 to
802d2dc
Compare
|
/runIntegrationTests |
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]> redid the constructors + extracted state Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
no longer needed that we have a single file for crash reporting Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
nkomonen-amazon
force-pushed
the
crashReporting
branch
from
802d2dc to
d6012a5
Compare
2 participants
Problem
We want to know when the extension crashes. Having telemetry will be useful.
The problem is that when the extension crashes it is not running anymore so it cannot report that it crashed.
Solution
Create a Crash Monitoring mechanism that has all the instances of an extension (Q/Toolkit) work together to monitor eachother for crashes, reporting of a crash if it is detected.
To review start with the main file
packages/core/src/shared/crashMonitoring/crashMonitoring.ts, specifically theCrashMonitoring.start()method.There is more information in the docstring of the class.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.