-
Notifications
You must be signed in to change notification settings - Fork 0
Integrate NoosphereService with Sentry logging #922
Integrate NoosphereService with Sentry logging #922
Conversation
@@ -74,14 +71,15 @@ actor NoosphereService: | |||
private var _sphereIdentity: String? | |||
/// Memoized Sphere instance | |||
private var _sphere: Sphere? | |||
private var errorLoggingService: any ErrorLoggingServiceProtocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to know about Sentry, just that we have an error logging service.
try await self.noosphere().createSphere( | ||
ownerKeyName: ownerKeyName | ||
) | ||
try await errorLoggingService.capturing { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all public methods that call to Noosphere or Sphere FFI wrappers, we wrap those calls with errorLoggingService.capturing
. This will log any errors that happen as a side-effect, and then re-throw the error.
} | ||
|
||
/// Set a new default sphere | ||
func resetSphere(_ identity: String?) { | ||
logger.debug("Reset sphere identity: \(identity ?? "none")") | ||
logger.log("Reset sphere identity: \(identity ?? "none")") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increasing log level to "log" in a few places.
}) | ||
Future { | ||
try await self.version() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use our Future helper to expose publishers. This lets us call methods from NoosphereService
, rather than Sphere
or Noosphere
, ensuring we get the error logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a question
@@ -133,7 +133,8 @@ final class Tests_DataService: XCTestCase { | |||
let noosphere = NoosphereService( | |||
globalStorageURL: globalStorageURL, | |||
sphereStorageURL: sphereStorageURL, | |||
gatewayURL: GatewayURL("http://unavailable-gateway.fakewebsite") | |||
gatewayURL: GatewayURL("http://unavailable-gateway.fakewebsite"), | |||
errorLoggingService: SentryIntegration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this result in sentry errors being logged during testing? I believe sentry is globally disabled during testing but maybe we should stub out a dummy integration here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Yes, we should.
...to prevent timeout failure due to GH's slow test env.
...and use for tests
Log errors generated by the FFI wrappers to Sentry.