ronantakizawa/codereview-bench huggingface dataset:
Repository: open-telemetry/opentelemetry-java
File: context/src/test/java/io/opentelemetry/context/propagation/internal/ExtendedTextMapGetterTest.java (Java)

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

package io.opentelemetry.context.propagation.internal;

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

import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

public class ExtendedTextMapGetterTest {

  final ExtendedTextMapGetter<Void> nullGet =
      new ExtendedTextMapGetter<Void>() {
        @Override
        public Iterable<String> keys(Void carrier) {
          return ImmutableList.of("key");
        }

        @Nullable
        @Override
        public String get(@Nullable Void carrier, String key) {
          return null;
        }
      };

  final ExtendedTextMapGetter<Void> nonNullGet =
      new ExtendedTextMapGetter<Void>() {
        @Override
        public Iterable<String> keys(Void carrier) {
          return ImmutableList.of("key");
        }

        @Override
        public String get(@Nullable Void carrier, String key) {
          return "123";

Review comment (suggestion):
```suggestion
class ExtendedTextMapGetterTest {
```

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

package io.opentelemetry.context.propagation.internal;

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

import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

class ExtendedTextMapGetterTest {

  final ExtendedTextMapGetter<Void> nullGet =
      new ExtendedTextMapGetter<Void>() {
        @Override
        public Iterable<String> keys(Void carrier) {
          return ImmutableList.of("key");
        }

        @Nullable
        @Override
        public String get(@Nullable Void carrier, String key) {
          return null;
        }
      };

  final ExtendedTextMapGetter<Void> nonNullGet =
      new ExtendedTextMapGetter<Void>() {
        @Override
        public Iterable<String> keys(Void carrier) {
          return ImmutableList.of("key");
        }

        @Override
        public String get(@Nullable Void carrier, String key) {
          return "123";
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/main/java/io/opentelemetry/sdk/logs/ReadWriteLogRecord.java (Java)

Code before:
  /** Returns the timestamp at which the log record occurred, in epoch nanos. */
  @Override
  default long getTimestampEpochNanos() {
    return toLogRecordData().getTimestampEpochNanos();
  }

  /** Returns the timestamp at which the log record was observed, in epoch nanos. */
  @Override
  default long getObservedTimestampEpochNanos() {
    return toLogRecordData().getTimestampEpochNanos();
  }

  /** Return the span context for this log, or {@link SpanContext#getInvalid()} if unset. */
  @Override
  default SpanContext getSpanContext() {
    return toLogRecordData().getSpanContext();
  }

  /** Returns the severity for this log, or {@link Severity#UNDEFINED_SEVERITY_NUMBER} if unset. */
  @Override
  default Severity getSeverity() {
    return toLogRecordData().getSeverity();
  }

  /** Returns the severity text for this log, or null if unset. */
  @Nullable
  @Override
  default String getSeverityText() {
    return toLogRecordData().getSeverityText();
  }

  @Deprecated
  @Override
  @SuppressWarnings("deprecation") // Implementation of deprecated method
  default io.opentelemetry.sdk.logs.data.Body getBody() {
    return toLogRecordData().getBody();
  }

  /** Returns the {@link Value} representation of the log body, of null if unset. */
  @Nullable
  @Override
  default Value<?> getBodyValue() {
    return toLogRecordData().getBodyValue();
  }

  /** Returns the attributes for this log, or {@link Attributes#empty()} if unset. */
  @Override
  default Attributes getAttributes() {
    return toLogRecordData().getAttributes();
  }


Review comment (suggestion):
`ReadableSpan` also has a method for accessing a single attribute, which would be good to add here. 
```
@Nullable
<T> T getAttribute(AttributeKey<T> key);
```

Code after:
  }

  /** Return the span context for this log, or {@link SpanContext#getInvalid()} if unset. */
  default SpanContext getSpanContext() {
    return toLogRecordData().getSpanContext();
  }

  /** Returns the severity for this log, or {@link Severity#UNDEFINED_SEVERITY_NUMBER} if unset. */
  default Severity getSeverity() {
    return toLogRecordData().getSeverity();
  }

  /** Returns the severity text for this log, or null if unset. */
  @Nullable
  default String getSeverityText() {
    return toLogRecordData().getSeverityText();
  }

  /** Returns the {@link Value} representation of the log body, of null if unset. */
  @Nullable
  default Value<?> getBodyValue() {
    return toLogRecordData().getBodyValue();
  }

  /** Returns the attributes for this log, or {@link Attributes#empty()} if unset. */
  default Attributes getAttributes() {
    return toLogRecordData().getAttributes();
  }
}
Repository: open-telemetry/opentelemetry-java
File: all/build.gradle.kts (Kotlin)

Code before:
}

description = "OpenTelemetry All"
otelJava.moduleName.set("io.opentelemetry.all")

// Skip OWASP dependencyCheck task on test module
dependencyCheck {
  skip = true
}

val testTasks = mutableListOf<Task>()
val jarTasks = mutableListOf<Jar>()

dependencies {
  rootProject.subprojects.forEach { subproject ->
    // Generate aggregate coverage report for published modules that enable jacoco.
    subproject.plugins.withId("jacoco") {
      subproject.plugins.withId("maven-publish") {
        implementation(project(subproject.path)) {
          isTransitive = false
        }
        subproject.tasks.withType<Test>().configureEach {
          testTasks.add(this)
        }
        subproject.tasks.withType<Jar>().forEach {
          if (it.archiveClassifier.get().isEmpty() && !it.archiveFile.get().toString().contains("jmh")) {
            jarTasks.add(it)
          }
        }
      }
    }
  }

  testImplementation("com.tngtech.archunit:archunit-junit5")
}

val artifactsAndJarsFile = layout.buildDirectory.file("artifacts_and_jars.txt").get().asFile

var writeArtifactsAndJars = tasks.register("writeArtifactsAndJars") {

  dependsOn(jarTasks)
  artifactsAndJarsFile.parentFile.mkdirs()
  artifactsAndJarsFile.createNewFile()
  val content = jarTasks.stream()
    .map {
      it.archiveBaseName.get() + ":" + it.archiveFile.get().toString()
    }.collect(Collectors.joining("\n"))
  artifactsAndJarsFile.writeText(content)
}

tasks {

Review comment (suggestion):
```suggestion
          if (it.archiveClassifier.get().isEmpty() && !it.name.contains("jmh")) {
```

Code after:
description = "OpenTelemetry All"
otelJava.moduleName.set("io.opentelemetry.all")

// Skip OWASP dependencyCheck task on test module
dependencyCheck {
  skip = true
}

val testTasks = mutableListOf<Task>()
val jarTasks = mutableListOf<Jar>()

dependencies {
  rootProject.subprojects.forEach { subproject ->
    // Generate aggregate coverage report for published modules that enable jacoco.
    subproject.plugins.withId("jacoco") {
      subproject.plugins.withId("maven-publish") {
        implementation(project(subproject.path)) {
          isTransitive = false
        }
        subproject.tasks.withType<Test>().configureEach {
          testTasks.add(this)
        }
        subproject.tasks.withType<Jar>().forEach {
          if (it.archiveClassifier.get().isEmpty() && !it.name.contains("jmh")) {
            jarTasks.add(it)
          }
        }
      }
    }
  }

  testImplementation("com.tngtech.archunit:archunit-junit5")
}

// Custom task type for writing artifacts and jars - configuration cache compatible
abstract class WriteArtifactsAndJars : DefaultTask() {
  @get:Input
  abstract val artifactData: MapProperty<String, String>

  @get:OutputFile
  abstract val outputFile: RegularFileProperty

  @TaskAction
  fun writeFile() {
    val file = outputFile.get().asFile
    file.parentFile.mkdirs()
    val content = artifactData.get().entries.joinToString("\n") { (baseName, filePath) ->
      "$baseName:$filePath"
    }
    file.writeText(content)
  }
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogRecordData.java (Java)

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

package io.opentelemetry.sdk.logs;

import com.google.auto.value.AutoValue;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.Value;
import io.opentelemetry.api.logs.Severity;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.logs.data.LogRecordData;
import io.opentelemetry.sdk.logs.data.internal.ExtendedLogRecordData;
import io.opentelemetry.sdk.resources.Resource;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

@AutoValue
@AutoValue.CopyAnnotations
@Immutable
abstract class SdkLogRecordData implements LogRecordData, ExtendedLogRecordData {

  SdkLogRecordData() {}

  static SdkLogRecordData create(
      Resource resource,
      InstrumentationScopeInfo instrumentationScopeInfo,
      @Nullable String eventName,
      long epochNanos,
      long observedEpochNanos,
      SpanContext spanContext,
      Severity severity,
      @Nullable String severityText,
      @Nullable Value<?> body,
      Attributes attributes,
      int totalAttributeCount) {
    return new AutoValue_SdkLogRecordData(
        resource,
        instrumentationScopeInfo,
        epochNanos,
        observedEpochNanos,
        spanContext,
        severity,
        severityText,
        attributes,
        totalAttributeCount,

Review comment (refactor):
Is it necessary to implement both?
```suggestion
abstract class SdkLogRecordData implements ExtendedLogRecordData {
```

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

package io.opentelemetry.sdk.logs;

import com.google.auto.value.AutoValue;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.Value;
import io.opentelemetry.api.logs.Severity;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.logs.data.internal.ExtendedLogRecordData;
import io.opentelemetry.sdk.resources.Resource;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

@AutoValue
@AutoValue.CopyAnnotations
@Immutable
abstract class SdkLogRecordData implements ExtendedLogRecordData {

  SdkLogRecordData() {}

  static SdkLogRecordData create(
      Resource resource,
      InstrumentationScopeInfo instrumentationScopeInfo,
      @Nullable String eventName,
      long epochNanos,
      long observedEpochNanos,
      SpanContext spanContext,
      Severity severity,
      @Nullable String severityText,
      @Nullable Value<?> body,
      Attributes attributes,
      int totalAttributeCount) {
    return new AutoValue_SdkLogRecordData(
        resource,
        instrumentationScopeInfo,
        epochNanos,
        observedEpochNanos,
        spanContext,
        severity,
        severityText,
        attributes,
        totalAttributeCount,
        body,
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/test/java/io/opentelemetry/sdk/logs/LogLimitsTest.java (Java)

Code before:
import org.junit.jupiter.api.Test;

class LogLimitsTest {

  @Test
  void defaultLogLimits() {
    assertThat(LogLimits.getDefault().getMaxNumberOfAttributes()).isEqualTo(128);
    assertThat(LogLimits.getDefault().getMaxAttributeValueLength()).isEqualTo(Integer.MAX_VALUE);
  }

  @Test
  void updateLogLimits_All() {
    LogLimits logLimits =
        LogLimits.builder().setMaxNumberOfAttributes(8).setMaxAttributeValueLength(9).build();
    assertThat(logLimits.getMaxNumberOfAttributes()).isEqualTo(8);
    assertThat(logLimits.getMaxAttributeValueLength()).isEqualTo(9);

    // Preserves values
    LogLimits logLimitsDupe = logLimits.toBuilder().build();
    // Use reflective comparison to catch when new fields are added.
    assertThat(logLimitsDupe).usingRecursiveComparison().isEqualTo(logLimits);
  }

  @Test
  void invalidLogLimits() {
    assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(-1))
        .isInstanceOf(IllegalArgumentException.class);
    assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(-1))
        .isInstanceOf(IllegalArgumentException.class);
    assertThatThrownBy(() -> LogLimits.builder().setMaxAttributeValueLength(-1))
        .isInstanceOf(IllegalArgumentException.class);
  }
}


Review comment (suggestion):
Oops, the -1 now appears in here twice. Might be nice to add a test case that covers the fact that 0 is now valid (no exception thrown).

Code after:

import org.junit.jupiter.api.Test;

class LogLimitsTest {

  @Test
  void defaultLogLimits() {
    assertThat(LogLimits.getDefault().getMaxNumberOfAttributes()).isEqualTo(128);
    assertThat(LogLimits.getDefault().getMaxAttributeValueLength()).isEqualTo(Integer.MAX_VALUE);
  }

  @Test
  void updateLogLimits_All() {
    LogLimits logLimits =
        LogLimits.builder().setMaxNumberOfAttributes(8).setMaxAttributeValueLength(9).build();
    assertThat(logLimits.getMaxNumberOfAttributes()).isEqualTo(8);
    assertThat(logLimits.getMaxAttributeValueLength()).isEqualTo(9);

    // Preserves values
    LogLimits logLimitsDupe = logLimits.toBuilder().build();
    // Use reflective comparison to catch when new fields are added.
    assertThat(logLimitsDupe).usingRecursiveComparison().isEqualTo(logLimits);
  }

  @Test
  void invalidLogLimits() {
    assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(-1))
        .isInstanceOf(IllegalArgumentException.class);
    assertThatThrownBy(() -> LogLimits.builder().setMaxAttributeValueLength(-1))
        .isInstanceOf(IllegalArgumentException.class);
  }

  @Test
  void validLogLimits() {
    assertThatCode(() -> LogLimits.builder().setMaxNumberOfAttributes(0))
        .doesNotThrowAnyException();
    assertThatCode(() -> LogLimits.builder().setMaxAttributeValueLength(0))
        .doesNotThrowAnyException();
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java (Java)

Code before:
    this.maxExportBatchSize = maxExportBatchSize;
    return this;
  }

  /**
   * Sets the {@link MeterProvider} to use to collect metrics related to batch export. If not set,
   * 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 = Math.min(DEFAULT_MAX_EXPORT_BATCH_SIZE, maxQueueSize);
      logger.log(Level.FINE, "Using maxExportBatchSize: {0}", maxExportBatchSize);
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}


Review comment (performance):
could we make this log indicate that we're overriding the value for `maxExportBatchSize` since it was larger than `maxQueueSize`?

Code after:

  /**
   * Sets the {@link MeterProvider} to use to collect metrics related to batch export. If not set,
   * 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.FINE, "Using maxExportBatchSize: {0}", maxExportBatchSize);
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}
Repository: open-telemetry/opentelemetry-java
File: api/all/src/main/java/io/opentelemetry/api/internal/Utils.java (Java)

Code before:
@Immutable
public final class Utils {

  private Utils() {}

  /**
   * Throws an {@link IllegalArgumentException} if the argument is false. This method is similar to
   * {@code Preconditions.checkArgument(boolean, Object)} from Guava.
   *
   * @param isValid whether the argument check passed.
   * @param errorMessage the message to use for the exception.
   */
  public static void checkArgument(boolean isValid, String errorMessage) {
    if (!isValid) {
      throw new IllegalArgumentException(errorMessage);
    }
  }

  /**
   * Logs a warning message if the argument is false.
   *
   * @param logger the logger instance that writes message.
   * @param isValid whether the argument check passed.
   * @param warnMessage the message to use for the warning log.
   */
  public static void warnOnArgument(Logger logger, boolean isValid, String warnMessage) {
    if (!isValid) {
      logger.log(Level.WARNING, warnMessage);
    }
  }
}


Review comment (performance):
I saw we just inline this in BatchLogRecordPorcessorBuilder. Its pretty trivial to abstract, and adds additional [shared internal code](https://github.com/open-telemetry/opentelemetry-java/issues/6970), which we're trying to move away from.

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

package io.opentelemetry.api.internal;

import javax.annotation.concurrent.Immutable;

/**
 * General internal utility methods.
 *
 * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
 * at any time.
 */
@Immutable
public final class Utils {

  private Utils() {}

  /**
   * Throws an {@link IllegalArgumentException} if the argument is false. This method is similar to
   * {@code Preconditions.checkArgument(boolean, Object)} from Guava.
   *
   * @param isValid whether the argument check passed.
   * @param errorMessage the message to use for the exception.
   */
  public static void checkArgument(boolean isValid, String errorMessage) {
    if (!isValid) {
      throw new IllegalArgumentException(errorMessage);
    }
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java (Java)

Code before:
  }

  /**
   * Sets the {@link MeterProvider} to use to collect metrics related to batch export. If not set,
   * 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 = Math.min(DEFAULT_MAX_EXPORT_BATCH_SIZE, maxQueueSize);
      logger.log(Level.FINE, "Using maxExportBatchSize: {0}", maxExportBatchSize);
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}


Review comment (performance):
Don't we need to se `maxExportBatchSize = Math.min(maxExportBatchSize, maxQueueSize);`?

If the maxExportBatchSize is set to `DEFAULT_MAX_EXPORT_BATCH_SIZE` on line 167, then export won't be triggered by the queue filling up, as discussed [here](https://github.com/open-telemetry/opentelemetry-java/issues/6454#issuecomment-2620223241).

Code after:
   * Sets the {@link MeterProvider} to use to collect metrics related to batch export. If not set,
   * 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.FINE, "Using maxExportBatchSize: {0}", maxExportBatchSize);
    }
    return new BatchLogRecordProcessor(
        logRecordExporter,
        meterProvider,
        scheduleDelayNanos,
        maxQueueSize,
        maxExportBatchSize,
        exporterTimeoutNanos);
  }
}