Make ThrottlingLogger logging time configurable

[ellen-lau]

Issue:

Related to this issue in OpenLiberty, when Liberty is unable to connect to the OpenTelemetry Collector the OTel sdk logs messages to indicate that failure. Using the ThrottlingLogger, it logs the message once a minute, which seems like a lot.

Proposed Solution:

Have the ThrottlingLogger logging time be configurable to a specific time interval (to 1 minute, 1 hour, etc).

[jkwatson]

How would you imagine configuring this, in general? I'm not opposed to adding this feature to the ThrottlingLogger itself, but how would you then configure that in the SDK?

[gabgg71]

The code for SDK would look something like this:

// src/main/java/io/opentelemetry/exporter/otlp/internal/OtlpLogRecordExporterProvider.java
//....
public class OtlpLogRecordExporterProvider
    implements ConfigurableLogRecordExporterProvider, AutoConfigureListener {

  private final AtomicReference<MeterProvider> meterProviderRef =
      new AtomicReference<>(MeterProvider.noop());

  @Override
  public LogRecordExporter createExporter(ConfigProperties config) {
    String protocol = OtlpConfigUtil.getOtlpProtocol(DATA_TYPE_LOGS, config);

    if (protocol.equals(PROTOCOL_HTTP_PROTOBUF)) {
      OtlpHttpLogRecordExporterBuilder builder = httpBuilder();

      OtlpConfigUtil.configureOtlpExporterBuilder(
          DATA_TYPE_LOGS,
          config,
          builder::setEndpoint,
          builder::addHeader,
          builder::setCompression,
          builder::setTimeout,
          builder::setTrustedCertificates,
          builder::setClientTls,
          builder::setRetryPolicy,
          builder::setMemoryMode,
          builder::setThrottlingLoggerTimeUnit);
      builder.setMeterProvider(meterProviderRef::get);

      return builder.build();
    } else if (protocol.equals(PROTOCOL_GRPC)) {
      OtlpGrpcLogRecordExporterBuilder builder = grpcBuilder();

      OtlpConfigUtil.configureOtlpExporterBuilder(
          DATA_TYPE_LOGS,
          config,
          builder::setEndpoint,
          builder::addHeader,
          builder::setCompression,
          builder::setTimeout,
          builder::setTrustedCertificates,
          builder::setClientTls,
          builder::setRetryPolicy,
          builder::setMemoryMode,
          builder::setThrottlingLoggerTimeUnit);
      builder.setMeterProvider(meterProviderRef::get);

      return builder.build();
    }
    throw new ConfigurationException("Unsupported OTLP logs protocol: " + protocol);
  }
//...

// src/main/java/io/opentelemetry/exporter/otlp/http/logs/OtlpHttpLogRecordExporterBuilder.java
//....
public final class OtlpHttpLogRecordExporterBuilder {
//....
public OtlpHttpLogRecordExporterBuilder setThrottlingLoggerTimeUnit(TimeUnit throttlingLoggerTimeUnit) {
    delegate.setThrottlingLoggerTimeUnit(throttlingLoggerTimeUnit);
    return this;
  }
//...
}

// src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java
//....
public final class HttpExporterBuilder<T extends Marshaler>{
private TimeUnit throttlingLoggerTimeUnit = TimeUnit.MINUTES;
//.... 
public HttpExporterBuilder<T> setThrottlingLoggerTimeUnit(TimeUnit throttlingLoggerTimeUnit) {
    this.throttlingLoggerTimeUnit = throttlingLoggerTimeUnit;
    return this;
  }
//...
public HttpExporter<T> build() {
//...

return new HttpExporter<>(
        exporterName,
        type,
        httpSender,
        meterProviderSupplier,
        exportAsJson,
        this.throttlingLoggerTimeUnit);
}
//....

// src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java
//....
public final class HttpExporter<T extends Marshaler> {

  private static final Logger internalLogger = Logger.getLogger(HttpExporter.class.getName());

  private final ThrottlingLogger logger;
 //...

  public HttpExporter(
      String exporterName,
      String type,
      HttpSender httpSender,
      Supplier<MeterProvider> meterProviderSupplier,
      boolean exportAsJson,
      TimeUnit throttlingLoggerTimeUnit) {
    this.type = type;
    this.httpSender = httpSender;
    this.exporterMetrics =
        exportAsJson
            ? ExporterMetrics.createHttpJson(exporterName, type, meterProviderSupplier)
            : ExporterMetrics.createHttpProtobuf(exporterName, type, meterProviderSupplier);

    this.logger = new ThrottlingLogger(internalLogger, throttlingLoggerTimeUnit);
  }
//....

// src/main/java/io/opentelemetry/sdk/internal/ThrottlingLogger.java
public class ThrottlingLogger {
  private static final double RATE_LIMIT = 5;
  private static final double THROTTLED_RATE_LIMIT = 1;
  private TimeUnit rateTimeUnit = MINUTES;

  private final Logger delegate;
  private final AtomicBoolean throttled = new AtomicBoolean(false);
  private final RateLimiter fastRateLimiter;
  private final RateLimiter throttledRateLimiter;

  public ThrottlingLogger(Logger delegate) {
    this(delegate, Clock.getDefault());
  }
// new constructor
  public ThrottlingLogger(Logger delegate, TimeUnit rateTimeUnit) {
    this(delegate);
    this.rateTimeUnit = rateTimeUnit;
  }
//...

This would likely require corresponding schema changes OpenTelemetry Configuration repo.

[jkwatson]

The only thing you want to configure is the time unit? Not the interval itself?

[gabgg71]

You're right, it makes more sense to configure the interval. We can express the other time units just by adjusting the interval or configure both of them.

[the-clam]

Hi @jkwatson @gabgg71, I'm picking up this issue from Ellen. It seems like it makes the most sense to be able to configure both the interval and the time unit. Wondering what the best way to go around doing this?

[jkwatson]

Hi @jkwatson @gabgg71, I'm picking up this issue from Ellen. It seems like it makes the most sense to be able to configure both the interval and the time unit. Wondering what the best way to go around doing this?

I would start with a small PR that makes the interval and time unit for the ThrottlingLogger itself parameterized. Then we can figure out how to get that configured into the SDK, then we can figure out how to make it configurable via declarative configuration. Start small and build it little by little.

[the-clam]

@jkwatson PR created with new constructor that allows specifying the time interval and unit; original values were kept as a default.

[the-clam]

Accidentally cause issued with the original PR; new PR here for reference: #7562

[the-clam]

@jack-berg I spoke with my team and we discussed an easier solution for our related issue; perhaps if it's not preferable to expose the configuration values for the ThrottlingLogger log rates, we could change the default value of the TimeUnit to hours (so it would log once per hour instead of once per minute when throttled)? That way it would stop the flooding of the messages as well.

[jkwatson]

@jack-berg I spoke with my team and we discussed an easier solution for our related issue; perhaps if it's not preferable to expose the configuration values for the ThrottlingLogger log rates, we could change the default value of the TimeUnit to hours (so it would log once per hour instead of once per minute when throttled)? That way it would stop the flooding of the messages as well.

Once per hour would essentially turn off logging, as no one would ever notice the messages. I don't think that would be a good default for most users. If there are issues, we'd like to make sure people are aware of them. Once per minute doesn't seem like an onerous amount of logging, if there really is an issue going on that should be addressed.