Service application of client logger changes client default behaviour

[dmeehan1968]

TL;DR

effect-aws installs a client logger by default, which is not the default for AWS clients (no logger). This interferes with application level use of the log levels (ERROR, INFO, DEBUG etc), with little control to the developer.

Context

Each effect-aws service implementation passes a logger implementation to the AWS client instance.

This means that at INFO level, every client command outputs a log entry including the commands input and output. This creates more logging detail than is strictly necessary.

Opinion

I may have application logs at INFO level that detail a subset of what the command receives/returns. It also means I don't have to deal with the DynamoDbClient attribute types, as I generally work with DynamoDbDocument which is more analagous to my application models.

AWS clients don't have a logger by default, it has to be provided, and I generally don't have application and client logging unless I'm actively debugging a client problem. Whilst the client level logging is sometimes useful, I find this a lower level of log information than the application level. I'd prefer to 'reserve' INFO for the application level, and have the client's INFO logs to be emitted at DEBUG, and DEBUG at TRACE. Effectively pushing the client logs one or more levels lower as they are more detail than generally needed.

As noted on #104 I have a preference to use PowerToolsLogger's appendKeys (annotations) as its lets me attach application context, but only have the info emitted to the logs if something later logs. For example, if I'm running at ERROR level, I can include the incoming event and see that as part of any ERROR log, if any is emitted. If there are no logs, the annotation has no 'cost' in logging terms. The alternative is to always output the event at INFO, and switch the lambda logging to INFO to capture it. Except now I'm logging every incoming event, not just the erroring ones.

Service's like EventBridge are difficult to search the logs for as the Detail is a JSON string, which means its both harder to read in the logs, and harder to search for. If I wanted to log EventBridge messages, I'd put my own logs in so that I capture the JSON output, not the stringified version. As noted above for Dynamo, it lets me deal with actual command input/output and not the even lower level dynamo attributes.

Conclusion

This might well be beyond the scope of effect-aws, as logging IS difficult and whilst PowerTools provides some benefits over Console.log or the native CloudWatch API's, its not a panacea. Each to their own perhaps.

If remapping is problematic, then perhaps at least having a default layer than doesn't have client level logging. e.g DynamoDbDocument.Default and DynamoDbDocument.DefaultWithoutClientLogging or some such.

Personally I think effect-aws adding logging by default means its being opinionated over the AWS client defaults, which potentially makes adoption harder.

[floydspace]

Hey @dmeehan1968
I agree with you, I got similar feedback from other users, and myself over the time I noticed that client logs just pollute the log stream. Here is my idea how to do that:

• the defaultLayer will not have logging enabled (it's like making clinet instance without any arguments)
• the layer will additionally have optional param EventBridge.layer({ enableClientLogs: true }) which will add the logger
• the baseLayer remains, as it is the most low lever constructor..
  • which currently can be used to disable client logs EventBridge.baseLayer(() => new EventBridgeClient())

I analyzed your idea to decrease severity for all clinet logs, but in my oppinion it may lead to confusuion, like should WARN become ERROR and should ERROR become DEBUG, it is a bit odd.. or it should be selective?! so I don't like this idea - it brings additional cognitive burden

[dmeehan1968]

That seems like a clean solution to stop changing the default client behaviour.

As for the level mapping, perhaps in the layer options allow a map of client log levels to logger log levels?

It's equivalent to supplying one's own logger to the client with the same mapping, but less verbose. Also if the map doesn't contain the source level, it keeps it as-is.

[floydspace]

I actually can implement it like this:

EventBridge.layer({
  clientLogger: {
    info: Effect.logInfo,
    warn: Effect.logWarning,
    error: Effect.logError,
    debug: Effect.logDebug,
    trace: Effect.logTrace,
  },
}),

EventBridge.layer({
  clientLogger: true, // would do the same as above
})

so the one can remap as he likes, or use default

[dmeehan1968]

You seem to be proposing both a logger and clientLogger field on the config. This might also be ambiguous, but I think there is a way to extend the existing logger field.

I think in @effect-aws at the moment, each service has its own implementation of the logger interface but there is no common implementation (I guess the code gen just emits the same code as preamble for each service), something like:

{
  logger: {
    info: (...content: any[]) => Effect.logInfo(...content).pipe(Effect.runSync)
    // ...
}

If supplied, this requires all log levels to be specified (except trace which is optional). This could be extracted into a single definition, shared by services and exposed on the service class (e.g. EventBridge.clientLogger, or just an export from @effect-aws).

If desired, the caller can reference this in the layer call in order to enable client logging (as is the current case with the defaultLayer):

EventBridge.layer({
  logger: EventBridge.clientLogger
})

If the caller wants to remap client info calls to debug, then they could use a form such as:

EventBridge.layer({
  logger: {
    ...EventBridge.clientLogger,
    remap: {
      info: EventBridge.clientLogger.debug
    }
  }
})

I agree with your assessment that remapping all levels to one level lower doesn't necessarily make sense, at some point two need to be treated as one, and making errors -> warnings probably doesn't make sense either. But moving client 'info' logs down a level to make them distinct from app 'info' logs (and perhaps client 'debug' to 'trace'), adds clarity to the logs. When we deal with architectual layers, it seems to me that lower levels of details should be moved logically down in the log output.

// unfortunately AWS doesn't export the `Logger` interface, but we can extract it from `ClientDefaults`
import { ClientDefaults } from "@aws-sdk/client-eventbridge"

// create our own `Logger` interface that adds a `remap` option
interface Logger extends ClientDefaults['logger'] {
  remap: Partial<ClientDefaults['logger']>
}

// create our own service config type that extends the client config
interface EventBridgeServiceConfig extends EventBridgeClientConfig {
  logger?: Logger | true
}

// update our service interface to use the service config
class EventBridgeService {
  // change to existing
  layer: (config?: EventBridgeServiceConfig) => Layer.Layer<EventBridgeService, never, never>
  //...
}

Then within the layer() implementation:

function layer(config?: EventBridgeServiceConfig) {
  let logger: ClientDefault['logger'] | undefined

  if (typeof config?.logger !== undefined) {
    // do nothing
  } else  if (typeof config?.logger === 'boolean') {
    logger = EventBridgeService.clientLogger
  } else {
    let { error, warn, info, debug, trace, remap } = config.logger
    if (remap) {
      error &&= remap.error
      warn &&= remap.warn
      info &&= remap.info
      debug &&= remap.debug
      trace &&= remap.trace
    }
    logger = { error, warn, info, debug, trace }
  }

  // additonal layer setup, using the `logger` result
}

I think the current defaultLayer implementation sets a precedent for @effect-aws providing logging by default, and so changing that behaviour means a breaking change to the API. Some people could end up unintentionally losing logging behavior they are relying on.

I'm not sure how big a deal you feel that is, but you might want to consider keeping it that way, and using EventBridge.layer({ logger: undefined }) as the method to return the client behaviour to its default. If its changed, then this could just be signalled as a major release, but that also creates work for existing users. You might just have to accept that default logging is now what @effect-aws does, but provide a simple mechanism to opt out or adapt the behaviour.

As an aside, it took me a while to figure out that EventBridge.layer() allowed me a way to adapt the logger without also having to provide a client instance. Perhaps the docs could be enhanced in this regard, and also to highlight the logging behaviour and how to disable/adapt it.