ronantakizawa/codereview-bench huggingface dataset:
Repository: open-telemetry/opentelemetry-java
File: api/all/src/main/java/io/opentelemetry/api/logs/LogRecordBuilder.java (Java)

Code before:
   * Sets a Boolean attribute on the {@code LogRecord}. If the {@code LogRecord} previously
   * contained a mapping for the key, the old value is replaced by the specified value.
   *
   * <p>Note: It is strongly recommended to use {@link #setAttribute(AttributeKey, Object)}, and
   * pre-allocate your keys, if possible.
   *
   * @param key the key for this attribute.
   * @param value the value for this attribute.
   * @return this.
   */
  default LogRecordBuilder setAttribute(String key, Boolean value) {
    return setAttribute(booleanKey(key), value);
  }

  /**
   * Sets an Integer attribute on the {@code LogRecord}. If the {@code LogRecord} previously
   * contained a mapping for the key, the old value is replaced by the specified value.
   *
   * <p>Note: It is strongly recommended to use {@link #setAttribute(AttributeKey, Object)}, and
   * pre-allocate your keys, if possible.
   *
   * @param key the key for this attribute.
   * @param value the value for this attribute.
   * @return this.
   */
  default LogRecordBuilder setAttribute(String key, Integer value) {
    return setAttribute(key, value.longValue());
  }

  /** Emit the log record. */
  void emit();
}


Review comment (suggestion):
```suggestion
  default LogRecordBuilder setAttribute(String key, int value) {
```

Code after:
   * Sets a Boolean attribute on the {@code LogRecord}. If the {@code LogRecord} previously
   * contained a mapping for the key, the old value is replaced by the specified value.
   *
   * <p>Note: It is strongly recommended to use {@link #setAttribute(AttributeKey, Object)}, and
   * pre-allocate your keys, if possible.
   *
   * @param key the key for this attribute.
   * @param value the value for this attribute.
   * @return this.
   */
  default LogRecordBuilder setAttribute(String key, boolean value) {
    return setAttribute(booleanKey(key), value);
  }

  /**
   * Sets an Integer attribute on the {@code LogRecord}. If the {@code LogRecord} previously
   * contained a mapping for the key, the old value is replaced by the specified value.
   *
   * <p>Note: It is strongly recommended to use {@link #setAttribute(AttributeKey, Object)}, and
   * pre-allocate your keys, if possible.
   *
   * @param key the key for this attribute.
   * @param value the value for this attribute.
   * @return this.
   */
  default LogRecordBuilder setAttribute(String key, int value) {
    return setAttribute(key, (long) value);
  }

  /** Emit the log record. */
  void emit();
}
Repository: open-telemetry/opentelemetry-java
File: sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigurationBuilder.java (Java)

Code before:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.sdk.extension.incubator.fileconfig;

import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.OpenTelemetryConfigurationModel;
import java.util.function.Function;

public class DeclarativeConfigurationBuilder implements DeclarativeConfigurationCustomizer {
  private Function<OpenTelemetryConfigurationModel, OpenTelemetryConfigurationModel>
      modelCustomizer = Function.identity();

  @Override
  public void addModelCustomizer(
      Function<OpenTelemetryConfigurationModel, OpenTelemetryConfigurationModel> customizer) {
    modelCustomizer = mergeCustomizer(modelCustomizer, customizer);
  }

  private static <I, O1, O2> Function<I, O2> mergeCustomizer(
      Function<? super I, ? extends O1> first, Function<? super O1, ? extends O2> second) {
    return (I configured) -> {
      O1 firstResult = first.apply(configured);
      return second.apply(firstResult);
    };
  }

  public OpenTelemetryConfigurationModel customizeModel(
      OpenTelemetryConfigurationModel configurationModel) {
    return modelCustomizer.apply(configurationModel);
  }
}


Review comment (suggestion):
In the future, we might consider adding opt-in logging to print the out change in the model after each stage of customization, but I'm happy to come back to that type of thing later. This is a good start!

Code after:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.sdk.extension.incubator.fileconfig;

import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.OpenTelemetryConfigurationModel;
import java.util.function.Function;

/** Builder for the declarative configuration. */
public class DeclarativeConfigurationBuilder implements DeclarativeConfigurationCustomizer {
  private Function<OpenTelemetryConfigurationModel, OpenTelemetryConfigurationModel>
      modelCustomizer = Function.identity();

  @Override
  public void addModelCustomizer(
      Function<OpenTelemetryConfigurationModel, OpenTelemetryConfigurationModel> customizer) {
    modelCustomizer = mergeCustomizer(modelCustomizer, customizer);
  }

  private static <I, O1, O2> Function<I, O2> mergeCustomizer(
      Function<? super I, ? extends O1> first, Function<? super O1, ? extends O2> second) {
    return (I configured) -> {
      O1 firstResult = first.apply(configured);
      return second.apply(firstResult);
    };
  }

  /** Customize the configuration model. */
  public OpenTelemetryConfigurationModel customizeModel(
      OpenTelemetryConfigurationModel configurationModel) {
    return modelCustomizer.apply(configurationModel);
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java (Java)

Code before:
   * metrics will not be collected.
   */
  public BatchLogRecordProcessorBuilder setMeterProvider(MeterProvider meterProvider) {
    requireNonNull(meterProvider, "meterProvider");
    this.meterProvider = meterProvider;
    return this;
  }

  // Visible for testing
  int getMaxExportBatchSize() {
    return maxExportBatchSize;
  }

  /**
   * Returns a new {@link BatchLogRecordProcessor} that batches, then forwards them to the given
   * {@code logRecordExporter}.
   *
   * @return a new {@link BatchLogRecordProcessor}.
   */
  public BatchLogRecordProcessor build() {
    if (maxExportBatchSize > maxQueueSize) {
      maxExportBatchSize = maxQueueSize;
      logger.log(
          Level.WARNING,
          "maxExportBatchSize should not exceed maxQueueSize. Both are set to {0}.",
          maxQueueSize);
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}


Review comment (performance):
oh, sorry, I see the confusion now, what do you think of below (and not necessarily event mentioning that `maxExportBatchSize` was set, or keeping that but only at FINE)?

```suggestion
          "maxExportBatchSize ({0}) should not exceed maxQueueSize ({1})",
          maxExportBatchSize,
          maxQueueSize);
```

Code after:
   * metrics will not be collected.
   */
  public BatchLogRecordProcessorBuilder setMeterProvider(MeterProvider meterProvider) {
    requireNonNull(meterProvider, "meterProvider");
    this.meterProvider = meterProvider;
    return this;
  }

  // Visible for testing
  int getMaxExportBatchSize() {
    return maxExportBatchSize;
  }

  /**
   * Returns a new {@link BatchLogRecordProcessor} that batches, then forwards them to the given
   * {@code logRecordExporter}.
   *
   * @return a new {@link BatchLogRecordProcessor}.
   */
  public BatchLogRecordProcessor build() {
    if (maxExportBatchSize > maxQueueSize) {
      logger.log(
          Level.WARNING,
          "maxExportBatchSize should not exceed maxQueueSize. Setting maxExportBatchSize to {0} instead of {1}",
          new Object[] {maxQueueSize, maxExportBatchSize});
      maxExportBatchSize = maxQueueSize;
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}
Repository: open-telemetry/opentelemetry-java
File: context/src/main/java/io/opentelemetry/context/propagation/TextMapGetter.java (Java)

Code before:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.context.propagation;

import javax.annotation.Nullable;

/**
 * Interface that allows a {@code TextMapPropagator} to read propagated fields from a carrier.
 *
 * <p>{@code Getter} is stateless and allows to be saved as a constant to avoid runtime allocations.
 *
 * @param <C> carrier of propagation fields, such as an http request.
 */
public interface TextMapGetter<C> {

  /**
   * Returns all the keys in the given carrier.
   *
   * @param carrier carrier of propagation fields, such as an http request.
   * @since 0.10.0
   */
  Iterable<String> keys(C carrier);

  /**
   * Returns the first value of the given propagation {@code key} or returns {@code null}.
   *
   * @param carrier carrier of propagation fields, such as an http request.
   * @param key the key of the field.
   * @return the first value of the given propagation {@code key} or returns {@code null}.
   */
  @Nullable
  String get(@Nullable C carrier, String key);
}


Review comment (refactor):
I'm late to reviewing this, but it was brought to my attention due to https://github.com/micrometer-metrics/tracing/issues/1032. `Iterator` as a return type here is not typical in Java APIs. `Iterable` would be idiomatic if going for the lowest level of abstraction. An `Iterator` is a stateful iteration of an `Iterable`. In theory, the `Iterator` returned may not be at the first element of the `Iterable`. I'm not sure what the maintainers want to do given this has been released, but in my opinion, the API should not be left using `Iterator`.

Code after:
   * @since 0.10.0
   */
  Iterable<String> keys(C carrier);

  /**
   * Returns the first value of the given propagation {@code key} or returns {@code null}.
   *
   * @param carrier carrier of propagation fields, such as an http request.
   * @param key the key of the field.
   * @return the first value of the given propagation {@code key} or returns {@code null}.
   */
  @Nullable
  String get(@Nullable C carrier, String key);

  /**
   * If implemented, returns all values for a given {@code key} in order, or returns an empty list.
   *
   * <p>The default method returns the first value of the given propagation {@code key} as a
   * singleton list, or returns an empty list.
   *
   * @param carrier carrier of propagation fields, such as an http request.
   * @param key the key of the field.
   * @return all values for a given {@code key} in order, or returns an empty list. Default method
   *     wraps {@code get()} as an {@link Iterator}.
   */
  default Iterator<String> getAll(@Nullable C carrier, String key) {
    String first = get(carrier, key);
    if (first == null) {
      return Collections.emptyIterator();
    }
    return Collections.singleton(first).iterator();
  }
}
Repository: open-telemetry/opentelemetry-java
File: exporters/otlp/all/src/test/java/io/opentelemetry/exporter/otlp/http/metrics/OtlpHttpMetricExporterBuilderTest.java (Java)

Code before:

import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.sdk.common.export.MemoryMode;
import io.opentelemetry.sdk.metrics.export.AggregationTemporalitySelector;
import io.opentelemetry.sdk.metrics.export.DefaultAggregationSelector;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

public class OtlpHttpMetricExporterBuilderTest {

  @Test
  public void verifyToBuilderPreservesSettings() {
    AggregationTemporalitySelector temporalitySelector =
        Mockito.mock(AggregationTemporalitySelector.class);
    DefaultAggregationSelector defaultAggregationSelector =
        Mockito.mock(DefaultAggregationSelector.class);

    OtlpHttpMetricExporter original =
        OtlpHttpMetricExporter.builder()
            .setMemoryMode(MemoryMode.IMMUTABLE_DATA)
            .setAggregationTemporalitySelector(temporalitySelector)
            .setDefaultAggregationSelector(defaultAggregationSelector)
            .build();

    OtlpHttpMetricExporter copy = original.toBuilder().build();

    assertThat(copy.getMemoryMode()).isEqualTo(MemoryMode.IMMUTABLE_DATA);
    assertThat(copy.aggregationTemporalitySelector).isSameAs(temporalitySelector);
    assertThat(copy.defaultAggregationSelector).isSameAs(defaultAggregationSelector);
  }
}


Review comment (suggestion):
Should clean exporters afterwards to free the thread pools initialized by the underlying http client libraries

Code after:

import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.sdk.common.export.MemoryMode;
import io.opentelemetry.sdk.metrics.export.AggregationTemporalitySelector;
import io.opentelemetry.sdk.metrics.export.DefaultAggregationSelector;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

class OtlpHttpMetricExporterBuilderTest {

  @Test
  void verifyToBuilderPreservesSettings() {
    AggregationTemporalitySelector temporalitySelector =
        Mockito.mock(AggregationTemporalitySelector.class);
    DefaultAggregationSelector defaultAggregationSelector =
        Mockito.mock(DefaultAggregationSelector.class);

    OtlpHttpMetricExporter original =
        OtlpHttpMetricExporter.builder()
            .setMemoryMode(MemoryMode.IMMUTABLE_DATA)
            .setAggregationTemporalitySelector(temporalitySelector)
            .setDefaultAggregationSelector(defaultAggregationSelector)
            .build();

    OtlpHttpMetricExporter copy = original.toBuilder().build();

    assertThat(copy.getMemoryMode()).isEqualTo(MemoryMode.IMMUTABLE_DATA);
    assertThat(copy.aggregationTemporalitySelector).isSameAs(temporalitySelector);
    assertThat(copy.defaultAggregationSelector).isSameAs(defaultAggregationSelector);

    original.close();
    copy.close();
  }
}
Repository: open-telemetry/opentelemetry-java
File: api/all/src/main/java/io/opentelemetry/api/common/AttributeType.java (Java)

Code before:
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.api.common;

/**
 * An enum that represents all the possible value types for an {@code AttributeKey} and hence the
 * types of values that are allowed for {@link Attributes}.
 */
public enum AttributeType {
  STRING,
  BOOLEAN,
  LONG,
  DOUBLE,
  STRING_ARRAY,
  BOOLEAN_ARRAY,
  LONG_ARRAY,
  DOUBLE_ARRAY;

  /**
   * Returns whether {@code this} is a primitive type or not.
   *
   * @return {@code true} if primitive, otherwise {@code false}
   */
  public boolean isPrimitive() {
    if (AttributeType.STRING.equals(this)) {
      return true;
    } else if (AttributeType.BOOLEAN.equals(this)) {
      return true;
    } else if (AttributeType.LONG.equals(this)) {
      return true;
    } else if (AttributeType.DOUBLE.equals(this)) {
      return true;
    } else if (AttributeType.STRING_ARRAY.equals(this)) {
      return false;
    } else if (AttributeType.BOOLEAN_ARRAY.equals(this)) {
      return false;
    } else if (AttributeType.LONG_ARRAY.equals(this)) {
      return false;
    } else if (AttributeType.DOUBLE_ARRAY.equals(this)) {
      return false;
    } else {
      throw new IllegalStateException(("Unrecognized attribute type: " + this));
    }
  }
}


Review comment (performance):
This could be implemented as an `EnumSet#contains`, it would save some lines and could be slightly more efficient

Code after:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.api.common;

/**
 * An enum that represents all the possible value types for an {@code AttributeKey} and hence the
 * types of values that are allowed for {@link Attributes}.
 */
public enum AttributeType {
  STRING,
  BOOLEAN,
  LONG,
  DOUBLE,
  STRING_ARRAY,
  BOOLEAN_ARRAY,
  LONG_ARRAY,
  DOUBLE_ARRAY
}
Repository: open-telemetry/opentelemetry-java
File: exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/internal/PrometheusComponentProvider.java (Java)

Code before:
  @Override
  public Class<MetricReader> getType() {
    return MetricReader.class;
  }

  @Override
  public String getName() {
    return "prometheus";
  }

  @Override
  public MetricReader create(DeclarativeConfigProperties config) {
    PrometheusHttpServerBuilder prometheusBuilder = PrometheusHttpServer.builder();

    Integer port = config.getInt("port");
    if (port != null) {
      prometheusBuilder.setPort(port);
    }

    String host = config.getString("host");
    if (host != null) {
      prometheusBuilder.setHost(host);
    }

    Boolean withoutScopeInfo = config.getBoolean("without_scope_info");
    if (Objects.equals(withoutScopeInfo, true)) {
      prometheusBuilder.setOtelScopeMode(OtelScopeMode.LABELS_ONLY);
    }

    DeclarativeConfigProperties withResourceConstantLabels =
        config.getStructured("with_resource_constant_labels");
    if (withResourceConstantLabels != null) {
      List<String> included = withResourceConstantLabels.getScalarList("included", String.class);
      List<String> excluded = withResourceConstantLabels.getScalarList("excluded", String.class);
      if (included != null || excluded != null) {
        prometheusBuilder.setAllowedResourceAttributesFilter(
            IncludeExcludePredicate.createPatternMatching(included, excluded));
      }
    }

    return prometheusBuilder.build();
  }
}


Review comment (suggestion):
(just a style preference)
```suggestion
    if (Boolean.TRUE.equals(withoutScopeInfo)) {
```

Code after:
    return MetricReader.class;
  }

  @Override
  public String getName() {
    return "prometheus";
  }

  @Override
  public MetricReader create(DeclarativeConfigProperties config) {
    PrometheusHttpServerBuilder prometheusBuilder = PrometheusHttpServer.builder();

    Integer port = config.getInt("port");
    if (port != null) {
      prometheusBuilder.setPort(port);
    }

    String host = config.getString("host");
    if (host != null) {
      prometheusBuilder.setHost(host);
    }

    DeclarativeConfigProperties withResourceConstantLabels =
        config.getStructured("with_resource_constant_labels");
    if (withResourceConstantLabels != null) {
      List<String> included = withResourceConstantLabels.getScalarList("included", String.class);
      List<String> excluded = withResourceConstantLabels.getScalarList("excluded", String.class);
      if (included != null || excluded != null) {
        prometheusBuilder.setAllowedResourceAttributesFilter(
            IncludeExcludePredicate.createPatternMatching(included, excluded));
      }
    }

    return prometheusBuilder.build();
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/common/src/test/java/io/opentelemetry/sdk/internal/DaemonThreadFactoryTest.java (Java)

Code before:
          thread.setUncaughtExceptionHandler(defaultHandler);
          return thread;
        };
    ExecutorService service =
        Executors.newSingleThreadExecutor(new DaemonThreadFactory("test", delegateFactory));
    Runnable runnable =
        () -> {
          Thread.UncaughtExceptionHandler uncaughtExceptionHandler =
              Thread.currentThread().getUncaughtExceptionHandler();

          assertThat(uncaughtExceptionHandler)
              .isInstanceOf(DaemonThreadFactory.ManagedUncaughtExceptionHandler.class);

          Thread threadMock = mock();

          // Verify interrupted exception
          uncaughtExceptionHandler.uncaughtException(threadMock, new InterruptedException());
          verify(threadMock).interrupt();
          verifyNoInteractions(defaultHandler);

          // Verify delegate exception
          clearInvocations(threadMock, defaultHandler);
          IllegalStateException e = new IllegalStateException();
          uncaughtExceptionHandler.uncaughtException(threadMock, e);
          verify(defaultHandler).uncaughtException(threadMock, e);
        };

    service.submit(runnable).get(5, TimeUnit.SECONDS);
  }
}


Review comment (suggestion):
I think that doing all assertions/verifications in an async block when the main test body doesn't also verify the execution is a code smell.

I think there could be something at the end of the `Runnable` (AtomicBoolean, CountdownLatch, etc) that you can then verify in the main thread. That way the reader can easily determine that the runnable completed which means the assertions all ran and passed.

Code after:
          Thread thread = Executors.defaultThreadFactory().newThread(r);
          thread.setUncaughtExceptionHandler(defaultHandler);
          return thread;
        };
    ExecutorService service =
        Executors.newSingleThreadExecutor(new DaemonThreadFactory("test", delegateFactory));
    Callable<Boolean> callable =
        () -> {
          Thread.UncaughtExceptionHandler uncaughtExceptionHandler =
              Thread.currentThread().getUncaughtExceptionHandler();

          assertThat(uncaughtExceptionHandler)
              .isInstanceOf(DaemonThreadFactory.ManagedUncaughtExceptionHandler.class);

          Thread threadMock = mock();

          // Verify interrupted exception
          uncaughtExceptionHandler.uncaughtException(threadMock, new InterruptedException());
          verify(threadMock).interrupt();
          verifyNoInteractions(defaultHandler);

          // Verify delegate exception
          clearInvocations(threadMock, defaultHandler);
          IllegalStateException e = new IllegalStateException();
          uncaughtExceptionHandler.uncaughtException(threadMock, e);
          verify(defaultHandler).uncaughtException(threadMock, e);
          return true;
        };

    assertThat(service.submit(callable).get(5, TimeUnit.SECONDS)).isTrue();
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSamplerBuilder.java (Java)

Code before:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.sdk.extension.incubator.trace.samplers;

import java.util.ArrayList;
import java.util.List;

/** A builder for a composable rule-based sampler. */
public final class ComposableRuleBasedSamplerBuilder {
  private final List<SamplingRule> rules = new ArrayList<>();

  /**
   * Adds a rule to use the given {@link ComposableSampler} if the {@link SamplingPredicate}
   * matches.
   */
  public ComposableRuleBasedSamplerBuilder add(
      SamplingPredicate predicate, ComposableSampler sampler) {
    rules.add(ImmutableSamplingRule.create(predicate, sampler));
    return this;
  }

  /** Returns a {@link ComposableSampler} with the rules in this builder. */
  public ComposableSampler build() {
    return new ComposableRuleBasedSampler(rules);
  }
}


Review comment (refactor):
This is just an initial implementation. Presumably this should have at least a few helpers on it, `addAttributePattern` for example

Code after:
/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.sdk.extension.incubator.trace.samplers;

import java.util.ArrayList;
import java.util.List;

/** A builder for a composable rule-based sampler. */
public final class ComposableRuleBasedSamplerBuilder {
  private final List<SamplingRule> rules = new ArrayList<>();

  ComposableRuleBasedSamplerBuilder() {}

  /**
   * Adds a rule to use the given {@link ComposableSampler} if the {@link SamplingPredicate}
   * matches.
   */
  public ComposableRuleBasedSamplerBuilder add(
      SamplingPredicate predicate, ComposableSampler sampler) {
    rules.add(ImmutableSamplingRule.create(predicate, sampler));
    return this;
  }

  /** Returns a {@link ComposableSampler} with the rules in this builder. */
  public ComposableSampler build() {
    return new ComposableRuleBasedSampler(rules);
  }
}