Skip to content

Commit

Permalink
Suppress Caffeine exception logging
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rtyley committed Sep 6, 2023
1 parent 69ee44e commit d47733a
Showing 1 changed file with 17 additions and 7 deletions.
24 changes: 17 additions & 7 deletions core/src/main/scala/com/gu/etagcaching/fetching/Fetching.scala
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package com.gu.etagcaching.fetching

import com.gu.etagcaching.Loading
import com.gu.etagcaching.{Endo, Loading}
import com.gu.etagcaching.fetching.Fetching.DurationRecorder.Result
import com.gu.etagcaching.fetching.Fetching._

import java.time.{Duration, Instant}
import scala.concurrent.{ExecutionContext, Future}
import scala.concurrent.{CancellationException, ExecutionContext, Future, TimeoutException}
import scala.util.{Failure, Success, Try}

trait Fetching[K, Response] {
Expand Down Expand Up @@ -33,7 +33,15 @@ trait Fetching[K, Response] {

def keyOn[K2](f: K2 => K): Fetching[K2, Response] = KeyAdapter(this)(f)

def mapResponse[Response2](f: Response => Response2): Fetching[K, Response2] = ResponseMapper(this)(f)
def mapResponse[Response2](f: Response => Response2): Fetching[K, Response2] =
ResponseTransform(this)(successTransform = f)

def suppressExceptionLoggingIf(p: Throwable => Boolean): Fetching[K, Response] =
ResponseTransform(this)(identity, failureTransform = { throwable =>
if (p(throwable)) // this is admittedly a pretty horrible hack
new CancellationException(s"Suppressing Caffeine cache exception logging for '$throwable'")
else throwable // Caffeine will log this exception: https://github.com/ben-manes/caffeine/issues/597
})

def thenParsing[V](parse: Response => V): Loading[K, V] = Loading.by(this)(parse)
}
Expand Down Expand Up @@ -73,12 +81,14 @@ object Fetching {
underlying.fetchOnlyIfETagChanged(f(key), eTag)
}

private case class ResponseMapper[K, UnderlyingResponse, Response](underlying: Fetching[K, UnderlyingResponse])(f: UnderlyingResponse => Response)
extends Fetching[K, Response] {
private case class ResponseTransform[K, UnderlyingResponse, Response](underlying: Fetching[K, UnderlyingResponse])(
successTransform: UnderlyingResponse => Response,
failureTransform: Endo[Throwable] = identity
) extends Fetching[K, Response] {
override def fetch(key: K)(implicit ec: ExecutionContext): Future[ETaggedData[Response]] =
underlying.fetch(key).map(_.map(f))
underlying.fetch(key).transform(_.map(successTransform), failureTransform)

override def fetchOnlyIfETagChanged(key: K, eTag: String)(implicit ec: ExecutionContext): Future[Option[ETaggedData[Response]]] =
underlying.fetchOnlyIfETagChanged(key, eTag).map(_.map(_.map(f)))
underlying.fetchOnlyIfETagChanged(key, eTag).transform(_.map(_.map(successTransform)), failureTransform)
}
}

0 comments on commit d47733a

Please sign in to comment.