Skip to content
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

Add hook to customize the way logs are emitted #481

Merged
merged 2 commits into from Jun 1, 2021

Conversation

syzygydevelopment
Copy link
Contributor

This changes adds the ability to set a new handler on the RCLogger type. By default, the handler is the current implementation of NSLog that emits the framework description.

The log handler provides the parameters of the log level and the message, and then anyone providing a custom handler can interpret those parameters however they wish and/or redirect them to their own logging system.

@aboedo
Copy link
Member

aboedo commented Mar 11, 2021

Hi!! 👋 Thanks so much for opening!
We actually have this in our backlog, just didn't get to it yet. I think it's super neat and helpful for developers.

The one gotcha here is that making the code from PurchasesCoreSwift available when doing import Purchases is slightly trickier, and involves making a public header file, like this one, which is the public-facing side of Transaction.

So in order for this to work, we'd to make it available through RCPurchases.h. We'd also need to retain the log format, so instead of calling the logHandler directly, we'd separate the NSLog part from the message format, making it so that replacing the logHandler only replaces the NSLog part, but not the actual message format.

@syzygydevelopment
Copy link
Contributor Author

I'll take a look at the header issue.

We'd also need to retain the log format, so instead of calling the logHandler directly, we'd separate the NSLog part from the message format, making it so that replacing the logHandler only replaces the NSLog part, but not the actual message format.

If an adopter is specifying a custom log handler, they don't need (nor want) the Purchases-provided log format. They know by virtue of setting the log handler that the messages are coming from the Purchases framework, and getting the LogLevel as a separate value allows them to translate it into a different LogLevel type, such as this one in the swift-log package.

@aboedo
Copy link
Member

aboedo commented Mar 11, 2021

@syzygydevelopment that's a good point, I agree.

@syzygydevelopment
Copy link
Contributor Author

I've been looking at this and am struggling to see how exposing the level would be possible, due to the structure of the package. Do you have any suggestions on how it could be exposed?

@aboedo
Copy link
Member

aboedo commented Mar 12, 2021

sorry @syzygydevelopment I should have been more clear about how to do it.

And thinking about it a bit more it's unfortunately not trivial, with the biggest problem being that LogLevel is a Swift enum that lives in PurchasesCoreSwift, so porting it over to Objective-C is kinda annoying.

The way I'd do it is:

  • Create a new public method in RCPurchases.h:
+ (void)setLogHandler:(void (^)(NSInteger, NSString *))logHandler;
  • Create the corresponding method in RCPurchases.m, and have the implementation just bridge over to Swift:
+ (void)setLogHandler:(void (^)(NSInteger, NSString *))logHandler {
    [RCLogger setLogHandler:logHandler];
}

Then update Logger.swift's logHandler to take in an Int, because of the enum bridging thing:

@objc public static var logHandler: (Int, String) -> Void = { levelAsInt, message in
    guard let level = LogLevel(rawValue: levelAsInt) else { return }
    NSLog("[\(frameworkDescription)] - \(level.description()): \(message)")
}

...


@objc public static func log(level: LogLevel, message: String) {
    guard level != .debug || shouldShowDebugLogs else { return }
    logHandler(level.rawValue, message)
}

I can't think of a better way around the enum problem, that doesn't involve having RCPurchases.h import PurchasesCoreSwift directly (which would cause a lot of issues) or maybe doing 2 enums (one in objc and one in swift), like we did for Intro eligibility:

This is probably the best approach, although it's quite a bit more work.

This is way more complicated than it should be, but having a mixed language framework that works in cocoapods, spm, carthage, etc has been challenging, to say the least. I'm hoping to do some bigger refactors soon that will this kind of thing a lot easier.

I totally understand if you don't want to go ahead and do the full implementation yourself, since it's a bit convoluted. If that's the case I'm happy to merge this and use it as a base to build the rest with.
I'm really, really grateful that you opened up this PR, kicked off the discussion around it, and your suggested approach makes total sense.

Let me know what you think!

@syzygydevelopment
Copy link
Contributor Author

@aboedo I've found a good compromise:

  • I renamed the existing RCLogLevel enum to be RCInternalLogLevel, since it's only used by the Objective-C code in the Purchases target.
  • I duplicated the enum (as an NS_ENUM) in a new Objective-C header (RCLogger.h) and made a note about it having the same layout as the Swift version.
  • When a user sets the custom log handler, the handler is wrapped in another closure that does a simple type-cast between the two log types.

I verified that:

  • this compiles fine
  • this works as expected for Objective-C adopters
  • this works as expected for Swift adopters

Since this involved creating a new RCLogger type, I also took the liberty of deprecating the Purchases.debugLogsEnabled property in favor of an identical one on the RCLogger interface. Please let me know what you think.

@aboedo aboedo added this to the 4.0.0 milestone Mar 16, 2021
@aboedo aboedo self-assigned this Mar 16, 2021
@aboedo
Copy link
Member

aboedo commented Mar 16, 2021

@syzygydevelopment sounds great! And yeah, deprecating the old setDebugLogsEnabled sounds reasonable. I'll be merging this soon and fixing tests.
Thanks so much for building this! And for going the extra mile to provide a clean API. We have a major release coming up very soon, I'll make sure this makes it in.

Thanks again!

@aboedo
Copy link
Member

aboedo commented Jun 1, 2021

@syzygydevelopment sorry for the embarasingly long wait on this. It's been a very packed couple of months. I'm merging this now and fixing the tests in a separate PR.

Thanks again for doing this and for your patience!

@aboedo aboedo merged commit 6ee40e7 into RevenueCat:develop Jun 1, 2021
This was referenced Jun 1, 2021
@aboedo aboedo mentioned this pull request Jul 13, 2021
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.

None yet

2 participants