search

growthbook / growthbook-sdk-java

The Java SDK for GrowthBook (JVM, Android)
https://docs.growthbook.io/lib/java
MIT License
7 stars 10 forks source link

The SDK should use a logging library #23

Closed igorlovich closed 1 month ago

igorlovich commented 1 year ago

Currently the sdk uses standard out/error for logging (System.out en printStackTrace). It should use a library (slf4j tends to be the standard allowing the end user to provide the binding to the actual logging framework e.g. logback or log4j etc..) . I can provide a PR if there is an interest.

tinahollygb commented 1 year ago

Hi @igorlovich thanks for offering to submit a PR for it. That would be great! Let's figure out the best approach.

We are trying to keep our dependencies to a minimum to keep our distribution size down. The SDK ships to Android as well, so adding to clients' bundle size is something we're trying to avoid if we can. If it can be done in a way that does not add additional dependencies and can be supported on Android, or the added dependencies are small, I think that would be acceptable.

I think it probably makes the most sense to have it added to the GBContext, as the last argument so that it's supported in the context builder. The SDK should not fail if a logger is not provided.

Is the slf4j library you're suggesting compatible with both Android and the JVM? If it's small and simple, I think we would be open to that change, e.g. if it's only a set of interfaces.

Did you want to discuss your suggested change first before getting started? Thanks.

tinahollygb commented 1 year ago

Also, to follow up with the previous message, these are the example projects we'd test that logging works for:

igorlovich commented 1 year ago

I understand your concerns and will try to address them one by one

  1. The -api jar is mostly interfaces and the latest version is 62 kb in size.

  2. The library does not do any logging as such; it is a facade (Slf4j = Simple Logging Facade for Java). By including the appropriate binding you can use whatever logging framework you want e.g. slf4j-logback, slf4j-log4j etc.. etc..

  3. For android logging there is slf4j-android binding, or if the project is using logback / log4j then one of the specific binding can be used.

  4. If no binding is provided the logging operation is a no-op.

  5. If the user wants to maintain the current behavior (i.e. printing to stdout/stderr) then they would need to provide sl4j-simple binding (12k in size)

Does this answer all your questions ?

tinahollygb commented 1 year ago

Does this answer all your questions ?

@igorlovich yes! That seems like a good solution then, thanks for explaining it.

Feel free to proceed with that approach. Thanks again for offering!

tinahollygb commented 1 year ago

@igorlovich thanks for the logging contribution. I've tested it out in some of the examples. My results are below.

The testing scenario is creating a GBFeaturesRepository that tries to decrypt an encrypted payload with a bad decryption key. This would result in an error log that should go via the logging setup you've set up.

✅ Spring

This is ok and appears to be working out-of-the-box. We see the error log.

🔴 Ktor

This is not working out-of-the-box. Ktor uses Logback. We get the following:

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See https://www.slf4j.org/codes.html#noProviders for further details.
SLF4J: Class path contains SLF4J bindings targeting slf4j-api versions 1.7.x or earlier.
SLF4J: Ignoring binding found at [jar:file:/..../.gradle/caches/modules-2/files-2.1/ch.qos.logback/logback-classic/1.2.11/4741689214e9d1e8408b206506cbe76d1c6a7d60/logback-classic-1.2.11.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See https://www.slf4j.org/codes.html#ignoredBindings for an explanation.

The output is not helpful and should log the error about a bad decryption key.

🔴 Java CLI

This is a new, plain app without any framework. The SDK needs to work in the simplest contexts in Java.

We get the following output which is not helpful to users. The example implementation should log the error logging related to a bad decryption key.

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See https://www.slf4j.org/codes.html#noProviders for further details.

🟡 Android

This is not yet tested. I wanted to see it working for the others first before adding it to Android, but if you are able to work with Android and add it, that would be great.

If not, we will need some guidance about how we can set this sort of thing up. The default logger in Android is Log, I'm not sure if that's slf4j-compatible. Perhaps a good example here is to have the user create their own logging class that implements slf4j and provide it to the GrowthBook SDK somehow. That latter part seems unclear and abstracted from us.

We will not be able to add your logging changes to the trunk until we are sure it's supported on all platforms we are supporting.

You can see the branch in the examples repo where I am adding the logging: https://github.com/growthbook/examples/compare/java-logging

Are you able to fork this branch and implement example logging classes for all the examples? It seems like the log field on all classes is private and final so a user cannot simply add a logger instance after the fact and must implement it another way, I'm guessing via annotations.

The suggestion I had made to add it to the constructor and allow a user to pass in their logging instance seems simpler to reason with. The Lombok annotations do seem like a faster implementation but they abstract the implementation.

We will need to be able to show working support for loggers in all our Java and Kotlin examples in order to merge it to main.

Let us know if you can help with these changes. Thanks!

igorlovich commented 1 year ago

Slf4j is NOT a logging framework (https://www.slf4j.org/manual.html). Like I have mentioned, the api dependency will not (and cannot) do any logging by itself. If you want "original" behavior in the examples, where things get printed out to stdout/stderr you need to include the implementation 'org.slf4j:slf4j-simple:2.0.7'

I just used lombok, because it was already used in the project (e.g. @Data, @Builder etc..). The annotation is equivalent to just writing

private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(SomeClass.class);

It does not really make sense to inject this through the context, as this field would and should never change. The whole point of the library is to abstract away the different logging frameworks behind a standard api. You can't have users inject loggers as they all have different apis / signatures.

tinahollygb commented 1 year ago

@igorlovich yes I'm aware slf4j does not provide logging and is a facade to be implemented. Did you miss the above examples? It works in only one of them out of the box (Spring) since they implement whatever needs to be implemented already, which means for the others, it will hide the current logging which may be helpful. Let us know if you can help with those examples? What do users who are unfamiliar with slf4j need to do in those other frameworks?

igorlovich commented 1 year ago

KTOR

The example uses logback as the logging framework (which implements the slf4j api directly). The logback version is too old (over a year) and should be upgraded to 1.4.7. But if you want to keep using it, we can downgrade the slf4j-api to 1.7.36. The spring example works because spring boot version that you use 2.7.4 pulls in the 1.7 API jar already

CLI

This has no logging framework; and if you want to output to console implementation 'org.slf4j:slf4j-simple:$slf4j_version' dependency needs to be added

Android

I have no way to test this as I do not have an android env setup. But I don't see why this will not work given that the appropriate binding is included

micpalmia commented 10 months ago

Any update here @tinahollygb ?

Printing out to console is definitely something that should be avoided if possible, and the proposed solution is good :)

tinahollygb commented 9 months ago

Hi @micpalmia thanks for the message. Was thinking about this issue from your PR message too. We haven't had a chance to tackle this as we have other priorities, but if you know Android development and how to work with slf4j there are a couple of outstanding tasks we need to do before we can be sure this will not introduce regressions in people's projects. You can see the details in this PR which implements this branch of work that @igorlovich has contributed. We want to get this added, but we need to be sure people's Android and framework-less projects will continue to work, and having example implementations that implement this so they can see the logging messages would be great. Is this something you want to help with? You can fork the branch in this project: https://github.com/growthbook/examples/pull/46

Essentially, we need to make sure for each of the examples:

tim06927 commented 2 months ago

Hey @tinahollygb, this topic became a priority for us at idealo as well after a Java application suffered from logging problems. Happy if using custom dev time helps you to prioritize this topic.

mvsmasiero commented 2 months ago

Helo @tinahollygb, Do you have any predictions about this feature for the SDK?

vazarkevych commented 1 month ago

Hi, we have created a pull request with a fix for this issue.