Use Messenger for logging and telemetry #2173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fregante
force-pushed
the
F/messenger/no-app
branch
from
42f9461
to
1e3d72d
Compare
fregante
commented
Dec 18, 2021
Comment on lines
+110
to
+123
| export const recordLog = getNotifier("RECORD_LOG", bg); | ||
| export const recordError = getNotifier("RECORD_ERROR", bg); | ||
| export const recordEvent = getNotifier("RECORD_EVENT", bg); | ||
| export const getLoggingConfig = getMethod("GET_LOGGING_CONFIG", bg); | ||
| export const setLoggingConfig = getMethod("SET_LOGGING_CONFIG", bg); | ||
|
|
||
| export const traces = { | ||
| addEntry: getNotifier("ADD_TRACE_ENTRY", bg), | ||
| addExit: getNotifier("ADD_TRACE_EXIT", bg), | ||
| clear: getNotifier("CLEAR_TRACES", bg), | ||
| }; | ||
|
|
||
| export const initTelemetry = getNotifier("INIT_TELEMETRY", bg); | ||
| export const sendDeploymentAlert = getNotifier("SEND_DEPLOYMENT_ALERT", bg); |
There was a problem hiding this comment.
Test report
Working
- GET_LOGGING_CONFIG
- SET_LOGGING_CONFIG
- RECORD_EVENT
- ADD_TRACE_ENTRY
- ADD_TRACE_EXIT
- CLEAR_TRACES
- RECORD_ERROR
Seen
- INIT_TELEMETRY (old INIT_UID)
Unable to reproduce
- SEND_DEPLOYMENT_ALERT
- RECORD_LOG
Comment on lines
+110
to
+112
| export const recordLog = getNotifier("RECORD_LOG", bg); | ||
| export const recordError = getNotifier("RECORD_ERROR", bg); | ||
| export const recordEvent = getNotifier("RECORD_EVENT", bg); |
There was a problem hiding this comment.
One note about these, since I mentioned them as blockers in:
- https://github.com/pixiebrix/pixiebrix-app/issues/882
- https://github.com/pixiebrix/pixiebrix-app/issues/883
I found:
recordLogisn't used at all in the apprecordEventappears in the app, but it isn't calledrecordErrormay be called, but since this is a "notifier" it will not throw any errors:- Where are notification errors handled? webext-messenger#40
- If we do see errors, they will be async and we can address them as part of the 2 issues I opened in the app
fregante
commented
Dec 18, 2021
Comment on lines
-32
to
-35
| export function initTelemetry(): void { | ||
| void initUID().catch((error) => { | ||
| console.warn("Error initializing uid", { error }); | ||
| }); |
There was a problem hiding this comment.
I dropped this function because it was just a redirect of initUID. Now initUID is called initTelemetry instead
twschiller
reviewed
Dec 20, 2021
| @@ -54,5 +54,5 @@ async function _reportError( | |||
| console.error(error); | |||
| } | |||
|
|
|||
| await recordError(serializeError(selectError(error)), context, null); | |||
| recordError(serializeError(selectError(error)), context, null); | |||
|
|
twschiller
approved these changes
Dec 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR best reviewed without whitespace changes
Resolved TypeScript issue
Related to and blocked by:
For some reason, TypeScript is throwing an error when I import the
background/telemetry.tsfile inlogging.ts. This is actually solved/avoided by #2178 so that PR should be merged first.