Skip to content

Add network logging support #63

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Add network logging support #63

wants to merge 7 commits into from

Conversation

stevensJourney
Copy link
Contributor

Overview

This adds the ability to log PowerSync network requests. This is the Swift port of powersync-ja/powersync-kotlin#229.

This also removes the experimental ConnectionMethod options which have recently been removed from the Kotlin SDK.

CHANGELOG.md Outdated
* Added the ability to log PowerSync sync network requests.

```swift
struct InlineLogger: SyncRequestLogger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're not re-using LoggerProtocol for this? Could the SyncRequestLoggerConfiguration take a LoggerProtocol, a severity (probably defaulting to something verbose) and the log level and then post messages there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially considered reusing LoggerProtocol, but opted for the current SyncRequestLogger.

The PowerSync network logger is focused specifically on HTTP request/response data, and uses a custom "log-level" SyncRequestLogLevel which controls what type of network data is emitted (headers, body, etc.), rather than traditional severity levels like .debug, .info, etc.

If we used LoggerProtocol, users would have to provide: a logger instance, a SyncRequestLogLevel and a corresponding LoggerLevel to use for all network messages. That felt slightly more complex and potentially confusing. With SyncRequestLogger, we give devs full control to decide how to map messages to their own logging system or levels inside log(_:).

There might not be a strong use case for this, but SyncRequestLogger also makes it slightly easier to process network logs without the use of a traditional logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair. I wonder if there should be another constructor on SyncRequestLoggerConfiguration that takes the logger, SyncRequestLogLevel and LoggerLevel. That way users who already have a logger don't need to implement another interface, and those who want them separate can use SyncRequestLogger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I like that idea. I've implemented your suggestion of an additional constructor. I've also removed SyncRequestLogger for a single closure based approach.

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.

2 participants