I don't believe ScopedLoggingContexts should obviously accept null "no-op" arguments.

[hagbard]

I don't believe ScopedLoggingContexts should obviously accept null "no-op" arguments.

[hagbard]

At the very least, passing "null" to withTags() / withLogLevelMap() should be a synonym for passing Tags.empty() / LogLevelMap.create(OFF) to avoid accidentally allowing methods to (sometimes) be callable twice.

And someone would need to check that passing LogLevelMap.create(OFF) is properly short-circuited when contexts are merged.

EDIT: I just checked and it looks like it is NOT handled well. Having the no-op log level map pushes you into a non-optimal code path for every log statement.

I had always envisaged code which want to add a log level map would look something like:

  DebugLevel dbgLevel = getDebugLevelFrom(request);
  if (dbgLevel != null) {
    LogLevelMap levelMap = getLevelMap(dbg);
    Tags tags = Tags.of("request_id", getRequestIdFrom(request));
    try (var context = ScopedLoggingContexts.newContext().withLogLevelMap(levelMap).withTags(tags).install()) {
      logger.atFine().log("Enabling targeted debug logging (level=%s)", dbgLevel);
      filterChain.doFilter(request, response);
    }    
  } else {
    // No debug level given, so no additional logging and no context created.
    filterChain.doFilter(request, response);
  }

[hagbard]

My recommendation is:

1. Revert the change to withTags() and document that Tags.empty() exists.
2. Add a LogLevelMap.noOp() singleton getter method and document that in withLogLevelMap()
3. Add a isNoOp() method in LogLevelMap and test that when merging maps and (importantly) when deciding whether to flip the static flag which makes every log statement slower.

https://github.com/google/flogger/blob/master/grpc/src/main/java/com/google/common/flogger/grpc/GrpcContextDataProvider.java#L74C30-L74C30

I.e. change:
https://github.com/google/flogger/blob/master/grpc/src/main/java/com/google/common/flogger/grpc/GrpcContextData.java#L139
to:

if (logLevelMap != null && !logLevelMap.isNoOp()) ...

[cgdecker]

Thanks for catching this. I was initially dubious about allowing null like this until I saw that there were in fact other methods in the same class doing that, but I hadn't thought about Tags.empty() etc. as an alternative. I'll check with the person who sent the change about whether there's some particular reason that wouldn't work for them, but I imagine we can roll this back and make the changes you describe.

[cgdecker]

82362ff changes withTags back to requiring a non-null argument and makes withLogLevelMap once again not callable twice.

null is currently still allowed for the LogLevelMap, in part because as you point out it's important to avoid using a LogLevelMap at all when it isn't needed, even if it's a no-op instance, and those code paths are already handled by null. Having to check for both null and isNoOp() feels a bit awkward and error-prone (though we could mitigate that some with a helper method).

I could also potentially see doing something like adding a no-op LogLevelMap, but only checking it in withLogLevelMap and translating it to null immediately (while still disallowing a null argument and not allowing it to be called twice).

[hagbard]

I'd still be interested to know where someone has a dynamically set level map which might be "no level map". I don't disbelieve it's possible but I did think about that use case and it always seemed better to just not create a context at all in those cases.