Suppress Caffeine exception logging

[rtyley]

This is a hack, and it probably won't be able to fully conceal that fact.

The facia-scala-client, in normal use, is asked for collections that don't yet have collection JSON, and may never have it (apparently, if a collection has never been edited, its collection JSON won't exist). In the past, this would be handled by generating a FaciaNotFound response for com.gu.facia.client.S3Client.get() which ApiClient would cheerfully not throw an exception for:

https://github.com/guardian/facia-scala-client/blob/f9830fd286d409662be850ff441792ec03c1c2e2/facia-json/src/main/scala/com/gu/facia/client/ApiClient.scala#L25-L27

...so encountering the missing resource would not be logged as an exception, which is the desired behaviour, see guardian/facia-scala-client#32

However, as my attempt (guardian/facia-scala-client#287) to introduce ETag-caching & AWS SDK v2 support rejects the com.gu.facia.client.S3Client trait, in favour of adapting com.gu.facia.client.ApiClient, I need to provide that same exception-supressing behaviour.

The task is complicated because currently ETagCache holds a scaffeine cache with value type ETaggedData[V] - there's no successful response for the cache loader to return for keys that can denote 'resource missing' - any successful value must have an ETag for one thing, as ETaggedData requires it, but when a resource is missing, we have no ETag.

This commit tried to deal with the problem by replacing the S3Exception with a CancellationException, which caffeine special-cases as exception to not log:

ben-manes/caffeine@1e52b10

...however this gets even yuckier than we might have thought, because it means that facia-scala-client, as the consumer of etag-caching, has to know that it must now try to catch CancellationException, rather than S3Exception, when it is trying to do it's own suppression of exception-logging.

[ben-manes]

Can you use your logger to suppress or filter these away? For example a Logback filter could reject the log message for the Caffeine logger namespace if the exception matches your conditions. That's less hacky, but unfortunately it is an application concern which is undesirable if providing a library for others and you want to quietly suppress it for your users. There is very little logging in the library and while this one was contentious, the benefits for users outweighed the (usually) easily suppressed log spam.

[rtyley]

Hi Ben - thank you for your wonderful library!

For example a Logback filter could reject the log message for the Caffeine logger namespace if the exception matches your conditions. That's less hacky, but unfortunately it is an application concern which is undesirable if providing a library for others and you want to quietly suppress it for your users.

Yes, that is the undesirable part :/ There are 7 consumers of the facia-scala-client library at the Guardian, and although it's a relatively simple logging config change to filter out the messages, I feel bad that trying to introduce an improvement (introducing ETag-caching) means that additional logging config is necessary - and some of these are big apps, maybe sometimes they want to see Caffeine exceptions, for other uses of Caffeine, and wouldn't want a blanket logging ban.

There is very little logging in the library and while this one was contentious, the benefits for users outweighed the (usually) easily suppressed log spam.

Understood - I've seen in ben-manes/caffeine#227 (comment), ben-manes/caffeine#597 & ben-manes/caffeine#885 (comment) that the logging makes sense when exceptions are exceptional, and developers could easily forget to handle them.

Really, I think for this particular case, my key-value model in etag-caching is just inadequate to serving the facia-scala-client usage pattern where missing keys are frequently selected. I need to change the cache inside ETagCache from:

AsyncLoadingCache[K, ETaggedData[V]]

...to:

AsyncLoadingCache[K, Option[ETaggedData[V]]]

...I'll just need to deal with all the Options, and probably have a conditional expiration policy.

When I created this PR, it was mostly to record my investigation in this other direction (suppressing exceptions), not because I wanted to merge it, but because I wanted to remember my motivation for having to go the other way and stick all the Options in!

[rtyley]

Closed in favour of #56