-
Notifications
You must be signed in to change notification settings - Fork 245
fix(logger): replace PassThrough with Writable for log handling COMPASS-9736 #7222
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
Changes from 6 commits
d952737
075a733
533f97a
f039277
85d9edd
05dd5e5
43838ce
8ea0ba3
2e3cb29
f24620d
bc4b214
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1521,6 +1521,9 @@ const connectWithOptions = ( | |
| inflightConnection = (async () => { | ||
| const deviceAuthAbortController = new AbortController(); | ||
|
|
||
| let cancelled = false; | ||
| let started = false; | ||
| let succeeded = false; | ||
| try { | ||
| if ( | ||
| getCurrentConnectionStatus(getState(), connectionInfo.id) === | ||
|
|
@@ -1581,7 +1584,11 @@ const connectWithOptions = ( | |
| mongoLogId(1_001_000_004), | ||
| 'Connection UI', | ||
| 'Initiating connection attempt', | ||
| { isAutoconnectAttempt } | ||
| { | ||
| isAutoconnectAttempt, | ||
| clusterName: connectionInfo.atlasMetadata?.clusterName, | ||
| connectionId: connectionInfo.id, | ||
| } | ||
| ); | ||
|
|
||
| // Connection form allows to start connecting with invalid connection | ||
|
|
@@ -1635,6 +1642,17 @@ const connectWithOptions = ( | |
| ); | ||
| } | ||
|
|
||
| started = true; | ||
| log.info( | ||
| mongoLogId(1_001_000_005), | ||
| 'Compass Connection Attempt Started', | ||
| 'Connection attempt started', | ||
| { | ||
| clusterName: connectionInfo.atlasMetadata?.clusterName, | ||
| connectionId: connectionInfo.id, | ||
| } | ||
| ); | ||
|
Comment on lines
+1639
to
+1647
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @syn-zhu On line 1583, there is log
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. related to my response to your next comment: Our purpose for introducing these logs was to facilitate connection latency tracking with Sentry. Given that, I wanted to reduce the surface area to connection attempts that would actually be meaningful. |
||
|
|
||
| const connectionAttempt = createConnectionAttempt({ | ||
| logger: log.unbound, | ||
| proxyOptions: proxyPreferenceToProxyOptions( | ||
|
|
@@ -1655,6 +1673,16 @@ const connectWithOptions = ( | |
| // This is how connection attempt indicates that the connection was | ||
| // aborted | ||
| if (!dataService || connectionAttempt.isClosed()) { | ||
| cancelled = true; | ||
| log.info( | ||
| mongoLogId(1_001_000_007), | ||
| 'Compass Connection Attempt Cancelled', | ||
| 'Connection attempt cancelled', | ||
| { | ||
| clusterName: connectionInfo.atlasMetadata?.clusterName, | ||
| connectionId: connectionInfo.id, | ||
| } | ||
| ); | ||
| dispatch({ | ||
| type: ActionTypes.ConnectionAttemptCancelled, | ||
| connectionId: connectionInfo.id, | ||
|
|
@@ -1821,6 +1849,17 @@ const connectWithOptions = ( | |
| connectionInfo | ||
| ); | ||
|
|
||
| succeeded = true; | ||
| log.info( | ||
| mongoLogId(1_001_000_006), | ||
| 'Compass Connection Attempt Succeeded', | ||
| 'Connection attempt succeeded', | ||
| { | ||
| clusterName: connectionInfo.atlasMetadata?.clusterName, | ||
| connectionId: connectionInfo.id, | ||
| } | ||
| ); | ||
|
|
||
| connectionProgress.openConnectionSucceededToast(connectionInfo); | ||
|
|
||
| // Emit before changing state because some plugins rely on this | ||
|
|
@@ -1856,6 +1895,18 @@ const connectWithOptions = ( | |
| ); | ||
| } | ||
| } catch (err) { | ||
| if (started && !succeeded && !cancelled) { | ||
syn-zhu marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| log.info( | ||
| mongoLogId(1_001_000_008), | ||
| 'Compass Connection Attempt Failed', | ||
| 'Connection attempt failed', | ||
| { | ||
| clusterName: connectionInfo.atlasMetadata?.clusterName, | ||
| connectionId: connectionInfo.id, | ||
| error: (err as Error).message, | ||
| } | ||
| ); | ||
| } | ||
| dispatch(connectionAttemptError(connectionInfo, err)); | ||
| } finally { | ||
| deviceAuthAbortController.abort(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import type { Logger } from '@mongodb-js/compass-logging/provider'; | ||
| import { MongoLogWriter } from 'mongodb-log-writer/mongo-log-writer'; | ||
| import { PassThrough } from 'stream'; | ||
| import type { Writable } from 'stream'; | ||
| import { mongoLogId } from '@mongodb-js/compass-logging/provider'; | ||
| import { useRef } from 'react'; | ||
|
|
||
|
|
@@ -62,13 +62,17 @@ export class CompassWebLogger implements Logger { | |
| }; | ||
| } | ||
| ) { | ||
| const passThrough = new PassThrough({ objectMode: true }); | ||
| this.log = new MongoLogWriter('', '', passThrough).bindComponent( | ||
| this.component | ||
| ); | ||
| passThrough.on('data', (line) => { | ||
| callbackRef.current.onLog?.(JSON.parse(line)); | ||
| }); | ||
| const target = { | ||
| write(line: string, callback: () => void) { | ||
| callbackRef.current.onLog?.(JSON.parse(line)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to update the test ensure it's happening synchronously?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added one! |
||
| callback(); | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| end(callback: () => void) { | ||
| callback(); | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| } as Writable; | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| this.log = new MongoLogWriter('', '', target).bindComponent(this.component); | ||
|
|
||
| this.debug = createCompassWebDebugger(this.component, this.callbackRef); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.