ably / ably-asset-tracking-android

Android client SDKs for the Ably Asset Tracking service.
Apache License 2.0
13 stars 6 forks source link

Logger / Logging Support #326

Open QuintinWillison opened 3 years ago

QuintinWillison commented 3 years ago

The obvious choice is SLF4J. In which case there should be no need for us to expose any additional API (beyond adding the slf4j-api mandatory dependency) to support logging as it'll be up the app developer to choose their own logging implementation (e.g. slf4j-simple during development) or even write their own.

Requirements:

We could consider using kotlin-logging on our implementation-side if it brings code-quality benefits for us, but it should not be necessary to expose this or any other Kotlin logging specialities transitively, or otherwise, through our API.

See Marco Behler's How To Do Logging In Java as it explains things very nicely for those unfamiliar with SLF4J.

The effective overhead to runtime APK size for app distributors should be assessed as part of this work - i.e. how big is an APK without SLF4J or any logging vs how big does it grow by once SLF4J API and simple implementations have been compiled in?

QuintinWillison commented 3 years ago

An additional requirement for work on this issue to be considered done is the production of some example code demonstrating how an app developer could write their own sink for log messages. The particular use case we have in mind is directing errors to Crashalytics. We should produce a prototype and a sample to demonstrate this, perhaps as a discrete demo project, given this use case is probably quite common.

See internal Slack thread.

KacperKluka commented 3 years ago

The example apps need to compile with the simple implementation

The simple logger logs to the console which isn't very useful in Android because we should be logging to the Logcat. Therefore, I've used the logback (namely logback-android) instead. It looks like the best choice for our use case because not only it supports Logcat out of the box, but also allows to create custom log handlers (called appenders) by just creating a class and adding it to the configuration file. :wink:

KacperKluka commented 3 years ago

APK size for app distributors should be assessed as part of this work

I've built debug versions of the publishing example app in two variants:

So we're increasing the app size by 0.2MB and this is without using a tool like proguard. After obfuscation and code shrinking we'd probably save some more space :wink:

kavalerov commented 3 years ago

Just to record internal discussion (https://ably-real-time.slack.com/archives/C01EPJENRD0/p1620922044288300 and below):What exactly is the purpose of this work, in terms of user value?

Things that don't allow for the SDK to work properly (like permission refusal) should probably be raised as exceptions, that are handled by the app developer. In what use case do we expect this logger to be used?

kavalerov commented 3 years ago

To answer my own comment from above, based on another internal discussion - the purpose of this work is to give better visibility into SDK operation including things that are not visible via exceptions due to the design of the SDK.

kavalerov commented 3 years ago

Based on the app size analysis, and on the benefits this interface brings to the users of the SDK I see no strong reasons to not move forward with landing this. 200kbs is not nothing, but doesn't sound like too much.

I did a quick research of how other libraries in the Java ecosystem do this, and sl4j seems to be a fairly standard approach. I want to do a deeper research, but don't have time for this right now. At the same time, I don't think this should block this functionality from landing.

@QuintinWillison @paddybyers are there any other concerns you have re this?

QuintinWillison commented 3 years ago

@kavalerov I, also, did some research before suggesting SLF4J - which is why I selected it as the obvious choice. I still stand by that assessment.

What I've since been discussing with @KacperKluka, albeit mostly via Slack DMs, is my desire to keep this as simple as possible in terms of added dependencies. As such I don't currently feel that additions such as kotlin-logging add enough value to effectively replace SLF4J in our top-level declared dependencies. For this initial cut we need it to be as obvious and flexible as possible, leaving as many technical choices to the customer as we can.

QuintinWillison commented 3 years ago

For the needs of a specific customer we've decided to proceed with https://github.com/ably/ably-asset-tracking-android/issues/347 in the short term as a lighter weight alternative to introducing SLF4J. This issue will remain open for us to work on a longer term solution that may or may not be based on SLF4J.

deanna-lad commented 1 year ago

The corresponding Jira issue is https://ably.atlassian.net/browse/SDK-809