Optimize log hot path (#4913)

jack-berg · web-flow · commit f8a4d81e2415 · 2022-11-08T17:05:00.000-06:00
* ComponentRegistry accepts name, version, schemaUrl instead of InstrumentationScopeInfo

* Fix comment
diff --git a/sdk/common/src/main/java/io/opentelemetry/sdk/internal/ComponentRegistry.java b/sdk/common/src/main/java/io/opentelemetry/sdk/internal/ComponentRegistry.java
@@ -5,46 +5,112 @@
 
 package io.opentelemetry.sdk.internal;
 
+import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 import java.util.function.Function;
+import javax.annotation.Nullable;
 
 /**
  * Component (tracer, meter, etc) registry class for all the provider classes (TracerProvider,
  * MeterProvider, etc.).
  *
+ * <p>Components are identified by name, version, and schema. Name is required, but version and
+ * schema are optional. Therefore, we have 4 possible scenarios for component keys:
+ *
+ * <ol>
+ *   <li>Only name is provided, represented by {@link #componentByName}
+ *   <li>Name and version are provided, represented by {@link #componentByNameAndVersion}
+ *   <li>Name and schema are provided, represented by {@link #componentByNameAndSchema}
+ *   <li>Name, version and schema are provided, represented by {@link
+ *       #componentByNameVersionAndSchema}
+ * </ol>
+ *
  * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
  * at any time.
  *
  * @param <V> the type of the registered value.
  */
 public final class ComponentRegistry<V> {
 
-  private final ConcurrentMap<InstrumentationScopeInfo, V> registry = new ConcurrentHashMap<>();
+  private final Map<String, V> componentByName = new ConcurrentHashMap<>();
+  private final Map<String, Map<String, V>> componentByNameAndVersion = new ConcurrentHashMap<>();
+  private final Map<String, Map<String, V>> componentByNameAndSchema = new ConcurrentHashMap<>();
+  private final Map<String, Map<String, Map<String, V>>> componentByNameVersionAndSchema =
+      new ConcurrentHashMap<>();
+
+  private final Set<V> allComponents = Collections.newSetFromMap(new IdentityHashMap<>());
+
   private final Function<InstrumentationScopeInfo, V> factory;
 
   public ComponentRegistry(Function<InstrumentationScopeInfo, V> factory) {
     this.factory = factory;
   }
 
   /**
-   * Returns the registered value associated with this {@link InstrumentationScopeInfo scope} if
-   * any, otherwise creates a new instance and associates it with the given scope.
+   * Returns the component associated with the {@code name}, {@code version}, and {@code schemaUrl}.
+   * {@link Attributes} are not part of component identity. Behavior is undefined when different
+   * {@link Attributes} are provided where {@code name}, {@code version}, and {@code schemaUrl} are
+   * identical.
    */
-  public V get(InstrumentationScopeInfo instrumentationScopeInfo) {
-    // Optimistic lookup, before creating the new component.
-    V component = registry.get(instrumentationScopeInfo);
-    if (component != null) {
-      return component;
+  public V get(
+      String name, @Nullable String version, @Nullable String schemaUrl, Attributes attributes) {
+    if (version != null && schemaUrl != null) {
+      Map<String, Map<String, V>> componentByVersionAndSchema =
+          componentByNameVersionAndSchema.computeIfAbsent(
+              name, unused -> new ConcurrentHashMap<>());
+      Map<String, V> componentBySchema =
+          componentByVersionAndSchema.computeIfAbsent(version, unused -> new ConcurrentHashMap<>());
+      return componentBySchema.computeIfAbsent(
+          schemaUrl,
+          schemaUrl1 ->
+              buildComponent(
+                  InstrumentationScopeInfo.builder(name)
+                      .setVersion(version)
+                      .setSchemaUrl(schemaUrl1)
+                      .setAttributes(attributes)
+                      .build()));
+    } else if (version != null) { // schemaUrl == null
+      Map<String, V> componentByVersion =
+          componentByNameAndVersion.computeIfAbsent(name, unused -> new ConcurrentHashMap<>());
+      return componentByVersion.computeIfAbsent(
+          version,
+          version1 ->
+              buildComponent(
+                  InstrumentationScopeInfo.builder(name)
+                      .setVersion(version1)
+                      .setAttributes(attributes)
+                      .build()));
+    }
+    if (schemaUrl != null) { // version == null
+      Map<String, V> componentBySchema =
+          componentByNameAndSchema.computeIfAbsent(name, unused -> new ConcurrentHashMap<>());
+      return componentBySchema.computeIfAbsent(
+          schemaUrl,
+          schemaUrl1 ->
+              buildComponent(
+                  InstrumentationScopeInfo.builder(name)
+                      .setSchemaUrl(schemaUrl1)
+                      .setAttributes(attributes)
+                      .build()));
+    } else { // schemaUrl == null && version == null
+      return componentByName.computeIfAbsent(
+          name,
+          name1 ->
+              buildComponent(
+                  InstrumentationScopeInfo.builder(name1).setAttributes(attributes).build()));
     }
+  }
 
-    V newComponent = factory.apply(instrumentationScopeInfo);
-    V oldComponent = registry.putIfAbsent(instrumentationScopeInfo, newComponent);
-    return oldComponent != null ? oldComponent : newComponent;
+  private V buildComponent(InstrumentationScopeInfo instrumentationScopeInfo) {
+    V component = factory.apply(instrumentationScopeInfo);
+    allComponents.add(component);
+    return component;
   }
 
   /**
@@ -53,6 +119,6 @@ public V get(InstrumentationScopeInfo instrumentationScopeInfo) {
    * @return a {@code Collection} view of the registered components.
    */
   public Collection<V> getComponents() {
-    return Collections.unmodifiableCollection(new ArrayList<>(registry.values()));
+    return Collections.unmodifiableCollection(allComponents);
   }
 }
diff --git a/sdk/common/src/test/java/io/opentelemetry/sdk/internal/ComponentRegistryTest.java b/sdk/common/src/test/java/io/opentelemetry/sdk/internal/ComponentRegistryTest.java
@@ -8,7 +8,6 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import io.opentelemetry.api.common.Attributes;
-import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
 import org.junit.jupiter.api.Test;
 
 class ComponentRegistryTest {
@@ -22,83 +21,35 @@ class ComponentRegistryTest {
 
   @Test
   void get_SameInstance() {
-    assertThat(registry.get(InstrumentationScopeInfo.builder(NAME).build()))
-        .isSameAs(registry.get(InstrumentationScopeInfo.builder(NAME).build()));
-    assertThat(registry.get(InstrumentationScopeInfo.builder(NAME).setVersion(VERSION).build()))
-        .isSameAs(registry.get(InstrumentationScopeInfo.builder(NAME).setVersion(VERSION).build()));
-    assertThat(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setSchemaUrl(SCHEMA_URL).build()))
+    assertThat(registry.get(NAME, null, null, Attributes.empty()))
+        .isSameAs(registry.get(NAME, null, null, Attributes.empty()))
+        .isSameAs(registry.get(NAME, null, null, Attributes.builder().put("k1", "v2").build()));
+
+    assertThat(registry.get(NAME, VERSION, null, Attributes.empty()))
+        .isSameAs(registry.get(NAME, VERSION, null, Attributes.empty()))
+        .isSameAs(registry.get(NAME, VERSION, null, Attributes.builder().put("k1", "v2").build()));
+    assertThat(registry.get(NAME, null, SCHEMA_URL, Attributes.empty()))
+        .isSameAs(registry.get(NAME, null, SCHEMA_URL, Attributes.empty()))
         .isSameAs(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setSchemaUrl(SCHEMA_URL).build()));
-    assertThat(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setAttributes(ATTRIBUTES).build()))
+            registry.get(NAME, null, SCHEMA_URL, Attributes.builder().put("k1", "v2").build()));
+    assertThat(registry.get(NAME, VERSION, SCHEMA_URL, Attributes.empty()))
+        .isSameAs(registry.get(NAME, VERSION, SCHEMA_URL, Attributes.empty()))
         .isSameAs(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setAttributes(ATTRIBUTES).build()));
-    assertThat(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME)
-                    .setVersion(VERSION)
-                    .setSchemaUrl(SCHEMA_URL)
-                    .setAttributes(ATTRIBUTES)
-                    .build()))
-        .isSameAs(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME)
-                    .setVersion(VERSION)
-                    .setSchemaUrl(SCHEMA_URL)
-                    .setAttributes(ATTRIBUTES)
-                    .build()));
+            registry.get(NAME, VERSION, SCHEMA_URL, Attributes.builder().put("k1", "v2").build()));
   }
 
   @Test
   void get_DifferentInstance() {
-    InstrumentationScopeInfo allFields =
-        InstrumentationScopeInfo.builder(NAME)
-            .setVersion(VERSION)
-            .setSchemaUrl(SCHEMA_URL)
-            .setAttributes(ATTRIBUTES)
-            .build();
+    assertThat(registry.get(NAME, VERSION, SCHEMA_URL, ATTRIBUTES))
+        .isNotSameAs(registry.get(NAME + "_1", VERSION, SCHEMA_URL, ATTRIBUTES))
+        .isNotSameAs(registry.get(NAME, VERSION + "_1", SCHEMA_URL, ATTRIBUTES))
+        .isNotSameAs(registry.get(NAME, VERSION, SCHEMA_URL + "_1", ATTRIBUTES));
+
+    assertThat(registry.get(NAME, VERSION, null, Attributes.empty()))
+        .isNotSameAs(registry.get(NAME, null, null, Attributes.empty()));
 
-    assertThat(registry.get(allFields))
-        .isNotSameAs(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME + "_1")
-                    .setVersion(VERSION)
-                    .setSchemaUrl(SCHEMA_URL)
-                    .setAttributes(ATTRIBUTES)
-                    .build()));
-    assertThat(registry.get(allFields))
-        .isNotSameAs(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME)
-                    .setVersion(VERSION + "_1")
-                    .setSchemaUrl(SCHEMA_URL)
-                    .setAttributes(ATTRIBUTES)
-                    .build()));
-    assertThat(registry.get(allFields))
-        .isNotSameAs(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME)
-                    .setVersion(VERSION)
-                    .setSchemaUrl(SCHEMA_URL + "_1")
-                    .setAttributes(ATTRIBUTES)
-                    .build()));
-    assertThat(registry.get(allFields))
-        .isNotSameAs(
-            registry.get(
-                InstrumentationScopeInfo.builder(NAME)
-                    .setVersion(VERSION)
-                    .setSchemaUrl(SCHEMA_URL)
-                    .setAttributes(Attributes.builder().put("k1", "v2").build())
-                    .build()));
-    assertThat(registry.get(InstrumentationScopeInfo.builder(NAME).setVersion(VERSION).build()))
-        .isNotSameAs(registry.get(InstrumentationScopeInfo.builder(NAME).build()));
-    assertThat(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setSchemaUrl(SCHEMA_URL).build()))
-        .isNotSameAs(registry.get(InstrumentationScopeInfo.builder(NAME).build()));
-    assertThat(
-            registry.get(InstrumentationScopeInfo.builder(NAME).setAttributes(ATTRIBUTES).build()))
-        .isNotSameAs(registry.get(InstrumentationScopeInfo.builder(NAME).build()));
+    assertThat(registry.get(NAME, null, SCHEMA_URL, Attributes.empty()))
+        .isNotSameAs(registry.get(NAME, null, null, Attributes.empty()));
   }
 
   private static final class TestComponent {}
diff --git a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLoggerBuilder.java b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLoggerBuilder.java
@@ -5,21 +5,22 @@
 
 package io.opentelemetry.sdk.logs;
 
+import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.logs.LoggerBuilder;
-import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
-import io.opentelemetry.sdk.common.InstrumentationScopeInfoBuilder;
 import io.opentelemetry.sdk.internal.ComponentRegistry;
 import javax.annotation.Nullable;
 
 final class SdkLoggerBuilder implements LoggerBuilder {
 
   private final ComponentRegistry<SdkLogger> registry;
-  private final InstrumentationScopeInfoBuilder scopeBuilder;
+  private final String instrumentationScopeName;
+  @Nullable private String instrumentationScopeVersion;
+  @Nullable private String schemaUrl;
   @Nullable private String eventDomain;
 
   SdkLoggerBuilder(ComponentRegistry<SdkLogger> registry, String instrumentationScopeName) {
     this.registry = registry;
-    this.scopeBuilder = InstrumentationScopeInfo.builder(instrumentationScopeName);
+    this.instrumentationScopeName = instrumentationScopeName;
   }
 
   @Override
@@ -30,19 +31,21 @@ public LoggerBuilder setEventDomain(String eventDomain) {
 
   @Override
   public SdkLoggerBuilder setSchemaUrl(String schemaUrl) {
-    scopeBuilder.setSchemaUrl(schemaUrl);
+    this.schemaUrl = schemaUrl;
     return this;
   }
 
   @Override
   public SdkLoggerBuilder setInstrumentationVersion(String instrumentationScopeVersion) {
-    scopeBuilder.setVersion(instrumentationScopeVersion);
+    this.instrumentationScopeVersion = instrumentationScopeVersion;
     return this;
   }
 
   @Override
   public SdkLogger build() {
-    SdkLogger logger = registry.get(scopeBuilder.build());
+    SdkLogger logger =
+        registry.get(
+            instrumentationScopeName, instrumentationScopeVersion, schemaUrl, Attributes.empty());
     return eventDomain == null ? logger : logger.withEventDomain(eventDomain);
   }
 }
diff --git a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLoggerProvider.java b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLoggerProvider.java
@@ -5,6 +5,7 @@
 
 package io.opentelemetry.sdk.logs;
 
+import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.logs.Logger;
 import io.opentelemetry.api.logs.LoggerBuilder;
 import io.opentelemetry.api.logs.LoggerProvider;
@@ -17,6 +18,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.function.Supplier;
 import java.util.logging.Level;
+import javax.annotation.Nullable;
 
 /** SDK implementation for {@link LoggerProvider}. */
 public final class SdkLoggerProvider implements LoggerProvider, Closeable {
@@ -61,7 +63,8 @@ public static SdkLoggerProviderBuilder builder() {
    */
   @Override
   public Logger get(String instrumentationScopeName) {
-    return loggerBuilder(instrumentationScopeName).build();
+    return loggerComponentRegistry.get(
+        instrumentationNameOrDefault(instrumentationScopeName), null, null, Attributes.empty());
   }
 
   /**
@@ -75,11 +78,16 @@ public LoggerBuilder loggerBuilder(String instrumentationScopeName) {
     if (isNoopLogRecordProcessor) {
       return LoggerProvider.noop().loggerBuilder(instrumentationScopeName);
     }
+    return new SdkLoggerBuilder(
+        loggerComponentRegistry, instrumentationNameOrDefault(instrumentationScopeName));
+  }
+
+  private static String instrumentationNameOrDefault(@Nullable String instrumentationScopeName) {
     if (instrumentationScopeName == null || instrumentationScopeName.isEmpty()) {
       LOGGER.fine("Logger requested without instrumentation scope name.");
-      instrumentationScopeName = DEFAULT_LOGGER_NAME;
+      return DEFAULT_LOGGER_NAME;
     }
-    return new SdkLoggerBuilder(loggerComponentRegistry, instrumentationScopeName);
+    return instrumentationScopeName;
   }
 
   /**
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterBuilder.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterBuilder.java
@@ -5,36 +5,39 @@
 
 package io.opentelemetry.sdk.metrics;
 
+import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.metrics.Meter;
 import io.opentelemetry.api.metrics.MeterBuilder;
-import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
-import io.opentelemetry.sdk.common.InstrumentationScopeInfoBuilder;
 import io.opentelemetry.sdk.internal.ComponentRegistry;
+import javax.annotation.Nullable;
 
 class SdkMeterBuilder implements MeterBuilder {
 
   private final ComponentRegistry<SdkMeter> registry;
-  private final InstrumentationScopeInfoBuilder scopeBuilder;
+  private final String instrumentationScopeName;
+  @Nullable private String instrumentationScopeVersion;
+  @Nullable private String schemaUrl;
 
   SdkMeterBuilder(ComponentRegistry<SdkMeter> registry, String instrumentationScopeName) {
     this.registry = registry;
-    this.scopeBuilder = InstrumentationScopeInfo.builder(instrumentationScopeName);
+    this.instrumentationScopeName = instrumentationScopeName;
   }
 
   @Override
   public MeterBuilder setSchemaUrl(String schemaUrl) {
-    scopeBuilder.setSchemaUrl(schemaUrl);
+    this.schemaUrl = schemaUrl;
     return this;
   }
 
   @Override
   public MeterBuilder setInstrumentationVersion(String instrumentationScopeVersion) {
-    scopeBuilder.setVersion(instrumentationScopeVersion);
+    this.instrumentationScopeVersion = instrumentationScopeVersion;
     return this;
   }
 
   @Override
   public Meter build() {
-    return registry.get(scopeBuilder.build());
+    return registry.get(
+        instrumentationScopeName, instrumentationScopeVersion, schemaUrl, Attributes.empty());
   }
 }
diff --git a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkTracerBuilder.java b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkTracerBuilder.java
