ronantakizawa/codereview-bench huggingface dataset:
Repository: open-telemetry/opentelemetry-java
File: sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingPredicate.java (Java)

Code before:
 */

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

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;

/** A predicate for a composable sampler, indicating whether a set of sampling arguments matches. */
public interface SamplingPredicate {
  /** Returns whether this {@link SamplingPredicate} matches the given sampling arguments. */
  boolean matches(
      Context parentContext,
      String traceId,
      String name,
      SpanKind spanKind,
      Attributes attributes,
      List<LinkData> parentLinks);

  /**
   * Returns a description of the {@link SamplingPredicate}. This may be displayed on debug pages or
   * in the logs.
   */
  String getDescription();
}


Review comment (suggestion):
Thought long on whether this should be a `FunctionalInterface`, but I felt just like `Sampler`, this is too important to not require

Code after:
 */

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

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;

/**
 * A predicate for a composable sampler, indicating whether a set of sampling arguments matches.
 *
 * <p>While this can be implemented with lambda expressions, it is recommended to implement {@link
 * Object#toString()} as well with an explanation of the predicate for rendering in {@link
 * io.opentelemetry.sdk.trace.samplers.Sampler#getDescription()}.
 */
@FunctionalInterface
public interface SamplingPredicate {
  /** Returns whether this {@link SamplingPredicate} matches the given sampling arguments. */
  boolean matches(
      Context parentContext,
      String traceId,
      String name,
      SpanKind spanKind,
      Attributes attributes,
      List<LinkData> parentLinks);
}
Repository: open-telemetry/opentelemetry-java
File: sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingRule.java (Java)

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

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

/** A rule which returns a {@link ComposableSampler} to use when a predicate matches. */
public interface SamplingRule {

  /** The {@link SamplingPredicate} which indicates whether to use {@link #sampler()}. */
  SamplingPredicate predicate();

  /** The {@link ComposableSampler} to use when the rule matches. */
  ComposableSampler sampler();
}


Review comment (suggestion):
It looks like this can be internal since ComposableRuleBasedSamplerBuilder has an `add(SamplingPredicate, ComposableSampler)` method

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

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

/** A rule which returns a {@link ComposableSampler} to use when a predicate matches. */
interface SamplingRule {

  /** The {@link SamplingPredicate} which indicates whether to use {@link #sampler()}. */
  SamplingPredicate predicate();

  /** The {@link ComposableSampler} to use when the rule matches. */
  ComposableSampler sampler();
}
Repository: open-telemetry/opentelemetry-java
File: api/incubator/src/test/java/io/opentelemetry/api/incubator/ConfigProviderTest.java (Java)

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

package io.opentelemetry.api.incubator;

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

import io.opentelemetry.api.incubator.config.ConfigProvider;
import org.junit.jupiter.api.Test;

class ConfigProviderTest {

  @Test
  void noopEquality() {
    ConfigProvider noop = ConfigProvider.noop();
    assertThat(ConfigProvider.noop()).isSameAs(noop);
  }

  @Test
  void instrumentationConfigFallback() {
    ConfigProvider configProvider = ConfigProvider.noop();
    assertThat(configProvider.getInstrumentationConfig()).isNull();
    assertThat(configProvider.getInstrumentationConfig("servlet")).isNotNull();
    assertThat(configProvider.getGeneralInstrumentationConfig()).isNotNull();
  }
}


Review comment (suggestion):
this is surprising to me. I don't like that one of these is null and one of them is not. We should have a non-surprising API for this, IMO.

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

package io.opentelemetry.api.incubator;

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

import io.opentelemetry.api.incubator.config.ConfigProvider;
import org.junit.jupiter.api.Test;

class ConfigProviderTest {

  @Test
  void noopEquality() {
    ConfigProvider noop = ConfigProvider.noop();
    assertThat(ConfigProvider.noop()).isSameAs(noop);
  }

  @Test
  void instrumentationConfigFallback() {
    ConfigProvider configProvider = ConfigProvider.noop();
    assertThat(configProvider.getInstrumentationConfig()).isNotNull();
    assertThat(configProvider.getInstrumentationConfig("servlet")).isNotNull();
    assertThat(configProvider.getGeneralInstrumentationConfig()).isNotNull();
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/all/src/jmh/java/io/opentelemetry/sdk/SpanRecordBenchmark.java (Java)

Code before:
  @Measurement(iterations = 5, time = 1)
  public void record_MultipleThreads(BenchmarkState benchmarkState) {
    record(benchmarkState);
  }

  private static void record(BenchmarkState benchmarkState) {
    for (int i = 0; i < RECORDS_PER_INVOCATION; i++) {
      Span span = benchmarkState.tracer.spanBuilder("test span name").startSpan();
      for (int j = 0; j < benchmarkState.attributeKeys.size(); j++) {
        span.setAttribute(
            benchmarkState.attributeKeys.get(j), benchmarkState.attributeValues.get(j));
      }
      for (int j = 0; j < benchmarkState.exceptions.size(); j++) {
        span.recordException(benchmarkState.exceptions.get(j));
      }
      for (int j = 0; j < benchmarkState.linkContexts.size(); j++) {
        span.addLink(benchmarkState.linkContexts.get(j));
      }
      span.end();
    }
  }

  public enum SpanSize {
    SMALL(0, 0, 0),
    MEDIUM(10, 1, 0),
    LARGE(100, 10, 5);

    private final int attributes;
    private final int events;
    private final int links;

    SpanSize(int attributes, int events, int links) {
      this.attributes = attributes;
      this.events = events;
      this.links = links;
    }
  }
}


Review comment (performance):
Check this out: if we have individual parameters for the num attributes, num events, num links, we end up with combinatorial explosion and a lot of noise. What we really want to characterize is the performance of different sizes of spans, where a size is a composite of a variety of dimensions.

Code after:
  @Fork(1)
  @Warmup(iterations = 5, time = 1)
  @Measurement(iterations = 5, time = 1)
  @OperationsPerInvocation(RECORDS_PER_INVOCATION)
  public void record_MultipleThreads(BenchmarkState benchmarkState) {
    record(benchmarkState);
  }

  private static void record(BenchmarkState benchmarkState) {
    for (int i = 0; i < RECORDS_PER_INVOCATION; i++) {
      Span span = benchmarkState.tracer.spanBuilder("test span name").startSpan();
      for (int j = 0; j < benchmarkState.attributeKeys.size(); j++) {
        span.setAttribute(
            benchmarkState.attributeKeys.get(j), benchmarkState.attributeValues.get(j));
      }
      for (int j = 0; j < benchmarkState.exceptions.size(); j++) {
        span.recordException(benchmarkState.exceptions.get(j));
      }
      for (int j = 0; j < benchmarkState.linkContexts.size(); j++) {
        span.addLink(benchmarkState.linkContexts.get(j));
      }
      span.end();
    }
  }

  public enum SpanSize {
    SMALL(0, 0, 0),
    MEDIUM(10, 1, 0),
    LARGE(100, 10, 5);

    private final int attributes;
    private final int events;
    private final int links;

    SpanSize(int attributes, int events, int links) {
      this.attributes = attributes;
      this.events = events;
      this.links = links;
    }
  }
}
Repository: open-telemetry/opentelemetry-java
File: sdk/common/src/main/java/io/opentelemetry/sdk/common/export/HttpSender.java (Java)

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

package io.opentelemetry.sdk.common.export;

import io.opentelemetry.sdk.common.CompletableResultCode;
import java.util.function.Consumer;

/**
 * An abstraction for executing HTTP requests, allowing for implementations backed by different
 * client libraries.
 *
 * <p>While this interface is public, implementing a custom sender is generally not recommended. The
 * {@code opentelemetry-java} project provides built-in implementations that cover virtually all
 * cases.
 *
 * @see HttpSenderProvider
 */
public interface HttpSender {

  /**
   * Send an HTTP request, including any retry attempts. {@code onResponse} is called with the HTTP
   * response, either a success response or an error response after retries. {@code onError} is
   * called when the request could not be executed due to cancellation, connectivity problems, or
   * timeout.
   *
   * @param messageWriter the request body message writer
   * @param onResponse the callback to invoke with the HTTP response
   * @param onError the callback to invoke when the HTTP request could not be executed
   */
  void send(
      MessageWriter messageWriter, Consumer<HttpResponse> onResponse, Consumer<Throwable> onError);

  /** Shutdown the sender. */
  CompletableResultCode shutdown();
}


Review comment (suggestion):
Given that `HttpSender` has been around for quite some time, it's a little misleading to claim that this is `@since 1.59.0` but since it has moved I guess technically that's accurate. Hrmph.

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

package io.opentelemetry.sdk.common.export;

import io.opentelemetry.sdk.common.CompletableResultCode;
import java.util.function.Consumer;

/**
 * An abstraction for executing HTTP requests, allowing for implementations backed by different
 * client libraries.
 *
 * <p>While this interface is public, implementing a custom sender is generally not recommended. The
 * {@code opentelemetry-java} project provides built-in implementations that cover virtually all
 * cases.
 *
 * @see HttpSenderProvider
 * @since 1.59.0
 */
public interface HttpSender {

  /**
   * Send an HTTP request, including any retry attempts. {@code onResponse} is called with the HTTP
   * response, either a success response or an error response after retries. {@code onError} is
   * called when the request could not be executed due to cancellation, connectivity problems, or
   * timeout.
   *
   * @param messageWriter the request body message writer
   * @param onResponse the callback to invoke with the HTTP response
   * @param onError the callback to invoke when the HTTP request could not be executed
   */
  void send(
      MessageWriter messageWriter, Consumer<HttpResponse> onResponse, Consumer<Throwable> onError);

  /** Shutdown the sender. */
  CompletableResultCode shutdown();
}
Repository: open-telemetry/opentelemetry-java
File: api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java (Java)

Code before:
  }

  @Override
  public Iterable<String> keys(Map<String, String> carrier) {
    if (carrier == null) {
      return Collections.emptyList();
    }
    return carrier.keySet();
  }

  @Nullable
  @Override
  public String get(@Nullable Map<String, String> carrier, String key) {
    if (carrier == null || key == null) {
      return null;
    }
    // Spec recommends using uppercase and underscores for environment variable
    // names for maximum
    // cross-platform compatibility.
    String sanitizedKey = key.replace('.', '_').replace('-', '_').toUpperCase(Locale.ROOT);
    String value = carrier.get(sanitizedKey);
    if (value != null && !EnvironmentSetter.isValidHttpHeaderValue(value)) {
      logger.log(
          Level.FINE,
          "Ignoring environment variable '{0}': "
              + "value contains characters not valid in HTTP header fields per RFC 9110.",
          sanitizedKey);
      return null;
    }
    return value;
  }

  @Override
  public String toString() {
    return "EnvironmentGetter";
  }
}


Review comment (suggestion):
Since EnvironmentGetter reads from the environment, keeping a copy, shouldn't this be `toLowerCase` instead of `toUpperCase`? Upper case I don't think will end up matching header values since the spec for w3c for example is `traceparent` not `TRACEPARENT`. environment variables in the environment should be uppercase, _ separated, but to auto be mapped to the w3c spec the normalized to lower. 

Hopefully this makes sense, and hopefully I'm reading this right. I haven't touched Java in a long time.

Code after:
  public static EnvironmentGetter getInstance() {
    return INSTANCE;
  }

  @Override
  public Iterable<String> keys(Map<String, String> carrier) {
    if (carrier == null) {
      return Collections.emptyList();
    }
    List<String> result = new ArrayList<>(carrier.size());
    for (String key : carrier.keySet()) {
      result.add(key.toLowerCase(Locale.ROOT));
    }
    return result;
  }

  @Nullable
  @Override
  public String get(@Nullable Map<String, String> carrier, String key) {
    if (carrier == null || key == null) {
      return null;
    }
    // Spec recommends using uppercase and underscores for environment variable
    // names for maximum
    // cross-platform compatibility.
    String sanitizedKey = key.replace('.', '_').replace('-', '_').toUpperCase(Locale.ROOT);
    String value = carrier.get(sanitizedKey);
    if (value != null && !EnvironmentSetter.isValidHttpHeaderValue(value)) {
      logger.log(
          Level.FINE,
          "Ignoring environment variable '{0}': "
              + "value contains characters not valid in HTTP header fields per RFC 9110.",
          sanitizedKey);
      return null;
    }
    return value;
  }

  @Override
  public String toString() {
    return "EnvironmentGetter";
  }
}
Repository: openai/codex
File: codex-rs/protocol/src/config_types.rs (Rust)

Code before:
    ) -> Self {
        let settings = self.settings();
        let updated_settings = Settings {
            model: model.unwrap_or_else(|| settings.model.clone()),
            reasoning_effort: effort.unwrap_or(settings.reasoning_effort),
        };

        match self {
            CollaborationMode::Plan(_) => CollaborationMode::Plan(updated_settings),
            CollaborationMode::Collaborate(_) => CollaborationMode::Collaborate(updated_settings),
            CollaborationMode::Execute(_) => CollaborationMode::Execute(updated_settings),
            CollaborationMode::Custom(custom) => CollaborationMode::Custom(CustomSettings {
                settings: updated_settings,
                developer_instructions: developer_instructions
                    .or_else(|| custom.developer_instructions.clone()),
            }),
        }
    }
}

/// Settings for a collaboration mode.
#[derive(Clone, PartialEq, Eq, Hash, Debug, Serialize, Deserialize, JsonSchema, TS)]
pub struct Settings {
    pub model: String,
    pub reasoning_effort: Option<ReasoningEffort>,
}

/// Custom settings that extend base settings with developer instructions.
#[derive(Clone, PartialEq, Eq, Hash, Debug, Serialize, Deserialize, JsonSchema, TS)]
pub struct CustomSettings {
    pub settings: Settings,
    pub developer_instructions: Option<String>,
}

Review comment (suggestion):
(not hard-blocking but important IMO) This feels like we're going to end up duplicating most of `ConfigProfile` here - I wonder if we should just reuse / copy that approach

Code after:
    ) -> Self {
        let settings = self.settings();
        let updated_settings = Settings {
            model: model.unwrap_or_else(|| settings.model.clone()),
            reasoning_effort: effort.unwrap_or(settings.reasoning_effort),
            developer_instructions: developer_instructions
                .or_else(|| settings.developer_instructions.clone()),
        };

        match self {
            CollaborationMode::Plan(_) => CollaborationMode::Plan(updated_settings),
            CollaborationMode::Collaborate(_) => CollaborationMode::Collaborate(updated_settings),
            CollaborationMode::Execute(_) => CollaborationMode::Execute(updated_settings),
            CollaborationMode::Custom(_) => CollaborationMode::Custom(updated_settings),
        }
    }
}

/// Settings for a collaboration mode.
#[derive(Clone, PartialEq, Eq, Hash, Debug, Serialize, Deserialize, JsonSchema, TS)]
pub struct Settings {
    pub model: String,
    pub reasoning_effort: Option<ReasoningEffort>,
    pub developer_instructions: Option<String>,
}
Repository: openai/codex
File: sdk/typescript/src/codexOptions.ts (TypeScript)

Code before:
export type CodexConfigValue =
  | string
  | number
  | boolean
  | CodexConfigValue[]
  | CodexConfigObject;

export type CodexConfigObject = { [key: string]: CodexConfigValue };

export type CodexOptions = {
  codexPathOverride?: string;
  baseUrl?: string;
  apiKey?: string;
  /**
   * Additional `--config key=value` overrides to pass to the Codex CLI.
   *
   * Provide a JSON object and the SDK will flatten it into dotted paths and
   * serialize values as TOML literals so they are compatible with the CLI's
   * `--config` parsing.
   */
  config?: CodexConfigObject;
  /**
   * Environment variables passed to the Codex CLI process. When provided, the SDK
   * will not inherit variables from `process.env`.
   */
  env?: Record<string, string>;

Review comment (suggestion):
We should be able to provide a fully typed config interface and zod schema, including narrow enums of the permitted string literals in specific config positions. Can we push the model to do this?

For example, TypeScript can natively represent that sandbox has a fixed set of valid string literals values. Let's do that!

Code after:
export type CodexConfigValue = string | number | boolean | CodexConfigValue[] | CodexConfigObject;

export type CodexConfigObject = { [key: string]: CodexConfigValue };

export type CodexOptions = {
  codexPathOverride?: string;
  baseUrl?: string;
  apiKey?: string;
  /**
   * Additional `--config key=value` overrides to pass to the Codex CLI.
   *
   * Provide a JSON object and the SDK will flatten it into dotted paths and
   * serialize values as TOML literals so they are compatible with the CLI's
   * `--config` parsing.
   */
  config?: CodexConfigObject;
  /**
   * Environment variables passed to the Codex CLI process. When provided, the SDK
   * will not inherit variables from `process.env`.
   */
  env?: Record<string, string>;
};
Repository: openai/codex
File: codex-rs/arg0/src/lib.rs (Rust)

Code before:
            std::fs::write(
                &batch_script,
                format!(
                    r#"@echo off
"{}" {CODEX_APPLY_PATCH_ARG1} %*
"#,
                    exe.display()
                ),
            )?;
        }
    }

    #[cfg(unix)]
    const PATH_SEPARATOR: &str = ":";

    #[cfg(windows)]
    const PATH_SEPARATOR: &str = ";";

    let path_element = path.display();
    let updated_path_env_var = match std::env::var("PATH") {
        Ok(existing_path) => {
            format!("{path_element}{PATH_SEPARATOR}{existing_path}")
        }
        Err(_) => {
            format!("{path_element}")
        }
    };

    unsafe {
        std::env::set_var("PATH", updated_path_env_var);
    }

    Ok(Arg0PathEntryGuard::new(temp_dir, lock_file))
}

fn janitor_cleanup(temp_root: &Path) -> std::io::Result<()> {
    let entries = match std::fs::read_dir(temp_root) {
        Ok(entries) => entries,
        Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
        Err(err) => return Err(err),
    };

    for entry in entries.flatten() {
        let path = entry.path();
        if !path.is_dir() {
            continue;
        }

        // Skip the directory if locking fails or the lock is currently held.
        let Some(_lock_file) = try_lock_dir(&path)? else {
            continue;

Review comment (suggestion):
Maybe add a docstring that, in practice, we expect this to be fast because, assuming things are getting cleaned up correctly, the number of entries in `temp_root` should be small (one per active `codex` process).

Code after:
    }

    #[cfg(unix)]
    const PATH_SEPARATOR: &str = ":";

    #[cfg(windows)]
    const PATH_SEPARATOR: &str = ";";

    let path_element = path.display();
    let updated_path_env_var = match std::env::var("PATH") {
        Ok(existing_path) => {
            format!("{path_element}{PATH_SEPARATOR}{existing_path}")
        }
        Err(_) => {
            format!("{path_element}")
        }
    };

    unsafe {
        std::env::set_var("PATH", updated_path_env_var);
    }

    Ok(Arg0PathEntryGuard::new(temp_dir, lock_file))
}

fn janitor_cleanup(temp_root: &Path) -> std::io::Result<()> {
    let entries = match std::fs::read_dir(temp_root) {
        Ok(entries) => entries,
        Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
        Err(err) => return Err(err),
    };

    for entry in entries.flatten() {
        let path = entry.path();
        if !path.is_dir() {
            continue;
        }

        // Skip the directory if locking fails or the lock is currently held.
        let Some(_lock_file) = try_lock_dir(&path)? else {
            continue;
        };

        match std::fs::remove_dir_all(&path) {
            Ok(()) => {}
            // Expected TOCTOU race: directory can disappear after read_dir/lock checks.
            Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
            Err(err) => return Err(err),
        }
    }