-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an optimized Attributes implementation for instrumenter #3136
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
152 changes: 152 additions & 0 deletions
152
benchmark/src/jmh/java/io/opentelemetry/benchmark/InstrumenterBenchmark.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.benchmark; | ||
|
||
import io.opentelemetry.api.OpenTelemetry; | ||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; | ||
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor; | ||
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor; | ||
import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetAttributesExtractor; | ||
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; | ||
import java.net.InetSocketAddress; | ||
import java.util.concurrent.TimeUnit; | ||
import org.checkerframework.checker.nullness.qual.Nullable; | ||
import org.openjdk.jmh.annotations.Benchmark; | ||
import org.openjdk.jmh.annotations.BenchmarkMode; | ||
import org.openjdk.jmh.annotations.Fork; | ||
import org.openjdk.jmh.annotations.Measurement; | ||
import org.openjdk.jmh.annotations.Mode; | ||
import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
import org.openjdk.jmh.annotations.Scope; | ||
import org.openjdk.jmh.annotations.State; | ||
import org.openjdk.jmh.annotations.Warmup; | ||
|
||
@Fork(3) | ||
@Warmup(iterations = 10, time = 1) | ||
@Measurement(iterations = 5, time = 1) | ||
@OutputTimeUnit(TimeUnit.MICROSECONDS) | ||
@BenchmarkMode(Mode.AverageTime) | ||
@State(Scope.Thread) | ||
public class InstrumenterBenchmark { | ||
|
||
private static final Instrumenter<Void, Void> INSTRUMENTER = | ||
Instrumenter.<Void, Void>newBuilder( | ||
OpenTelemetry.noop(), | ||
"benchmark", | ||
HttpSpanNameExtractor.create(ConstantHttpAttributesExtractor.INSTANCE)) | ||
.addAttributesExtractor(ConstantHttpAttributesExtractor.INSTANCE) | ||
.addAttributesExtractor(new ConstantNetAttributesExtractor()) | ||
.newInstrumenter(); | ||
|
||
@Benchmark | ||
public Context start() { | ||
return INSTRUMENTER.start(Context.root(), null); | ||
} | ||
|
||
@Benchmark | ||
public Context startEnd() { | ||
Context context = INSTRUMENTER.start(Context.root(), null); | ||
INSTRUMENTER.end(context, null, null, null); | ||
return context; | ||
} | ||
|
||
static class ConstantHttpAttributesExtractor extends HttpAttributesExtractor<Void, Void> { | ||
static HttpAttributesExtractor<Void, Void> INSTANCE = new ConstantHttpAttributesExtractor(); | ||
|
||
@Override | ||
protected @Nullable String method(Void unused) { | ||
return "GET"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String url(Void unused) { | ||
return "https://opentelemetry.io/benchmark"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String target(Void unused) { | ||
return "/benchmark"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String host(Void unused) { | ||
return "opentelemetry.io"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String route(Void unused) { | ||
return "/benchmark"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String scheme(Void unused) { | ||
return "https"; | ||
} | ||
|
||
@Override | ||
protected @Nullable String userAgent(Void unused) { | ||
return "OpenTelemetryBot"; | ||
} | ||
|
||
@Override | ||
protected @Nullable Long requestContentLength(Void unused, @Nullable Void unused2) { | ||
return 100L; | ||
} | ||
|
||
@Override | ||
protected @Nullable Long requestContentLengthUncompressed(Void unused, @Nullable Void unused2) { | ||
return null; | ||
} | ||
|
||
@Override | ||
protected @Nullable String flavor(Void unused, @Nullable Void unused2) { | ||
return SemanticAttributes.HttpFlavorValues.HTTP_2_0; | ||
} | ||
|
||
@Override | ||
protected @Nullable String serverName(Void unused, @Nullable Void unused2) { | ||
return null; | ||
} | ||
|
||
@Override | ||
protected @Nullable String clientIp(Void unused, @Nullable Void unused2) { | ||
return null; | ||
} | ||
|
||
@Override | ||
protected @Nullable Integer statusCode(Void unused, Void unused2) { | ||
return 200; | ||
} | ||
|
||
@Override | ||
protected @Nullable Long responseContentLength(Void unused, Void unused2) { | ||
return 100L; | ||
} | ||
|
||
@Override | ||
protected @Nullable Long responseContentLengthUncompressed(Void unused, Void unused2) { | ||
return null; | ||
} | ||
} | ||
|
||
static class ConstantNetAttributesExtractor | ||
extends InetSocketAddressNetAttributesExtractor<Void, Void> { | ||
|
||
private static final InetSocketAddress ADDRESS = | ||
InetSocketAddress.createUnresolved("localhost", 8080); | ||
|
||
@Override | ||
public @Nullable InetSocketAddress getAddress(Void unused, @Nullable Void unused2) { | ||
return ADDRESS; | ||
} | ||
|
||
@Override | ||
public @Nullable String transport(Void unused) { | ||
return SemanticAttributes.NetTransportValues.IP_TCP; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 69 additions & 0 deletions
69
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/UnsafeAttributes.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter; | ||
|
||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.common.Attributes; | ||
import io.opentelemetry.api.common.AttributesBuilder; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
/** | ||
* The {@link AttributesBuilder} and {@link Attributes} used by the instrumentation API. We are able | ||
* to take advantage of the fact that we know our attributes builder cannot be reused to create | ||
* multiple Attributes instances. So we use just one storage for both the builder and attributes. A | ||
* couple of methods still require copying to satisfy the interface contracts, but in practice | ||
* should never be called by user code even though they can. | ||
*/ | ||
final class UnsafeAttributes extends HashMap<AttributeKey<?>, Object> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jkwatson It's your favorite pattern again :) |
||
implements Attributes, AttributesBuilder { | ||
|
||
// Attributes | ||
|
||
@SuppressWarnings("unchecked") | ||
@Override | ||
public <T> T get(AttributeKey<T> key) { | ||
return (T) super.get(key); | ||
} | ||
|
||
@Override | ||
public Map<AttributeKey<?>, Object> asMap() { | ||
return this; | ||
} | ||
|
||
// This can be called by user code in a RequestListener so copy. In practice, it should not be | ||
// called as there is no real use case. | ||
@Override | ||
public AttributesBuilder toBuilder() { | ||
return Attributes.builder().putAll(this); | ||
} | ||
|
||
// AttributesBuilder | ||
|
||
// This can be called by user code in an AttributesExtractor so copy. In practice, it should not | ||
// be called as there is no real use case. | ||
@Override | ||
public Attributes build() { | ||
return toBuilder().build(); | ||
} | ||
|
||
@Override | ||
public <T> AttributesBuilder put(AttributeKey<Long> key, int value) { | ||
return put(key, (long) value); | ||
} | ||
|
||
@Override | ||
public <T> AttributesBuilder put(AttributeKey<T> key, T value) { | ||
super.put(key, value); | ||
return this; | ||
} | ||
|
||
@Override | ||
public AttributesBuilder putAll(Attributes attributes) { | ||
attributes.forEach(this::put); | ||
return this; | ||
} | ||
} |
86 changes: 86 additions & 0 deletions
86
...src/test/java/io/opentelemetry/instrumentation/api/instrumenter/UnsafeAttributesTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter; | ||
|
||
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; | ||
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry; | ||
|
||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.common.Attributes; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class UnsafeAttributesTest { | ||
|
||
@Test | ||
void buildAndUse() { | ||
Attributes previous = | ||
new UnsafeAttributes().put("world", "earth").put("country", "japan").build(); | ||
|
||
UnsafeAttributes attributes = new UnsafeAttributes(); | ||
attributes.put(AttributeKey.stringKey("animal"), "cat"); | ||
attributes.put("needs_catnip", false); | ||
// Overwrites | ||
attributes.put("needs_catnip", true); | ||
attributes.put(AttributeKey.longKey("lives"), 9); | ||
attributes.putAll(previous); | ||
|
||
assertThat((Attributes) attributes) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "japan"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L)); | ||
|
||
Attributes built = attributes.build(); | ||
assertThat(built) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "japan"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L)); | ||
|
||
attributes.put("clothes", "fur"); | ||
assertThat((Attributes) attributes) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "japan"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L), | ||
attributeEntry("clothes", "fur")); | ||
|
||
// Unmodified | ||
assertThat(built) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "japan"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L)); | ||
|
||
Attributes modified = attributes.toBuilder().put("country", "us").build(); | ||
assertThat(modified) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "us"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L), | ||
attributeEntry("clothes", "fur")); | ||
|
||
// Unmodified | ||
assertThat((Attributes) attributes) | ||
.containsOnly( | ||
attributeEntry("world", "earth"), | ||
attributeEntry("country", "japan"), | ||
attributeEntry("animal", "cat"), | ||
attributeEntry("needs_catnip", true), | ||
attributeEntry("lives", 9L), | ||
attributeEntry("clothes", "fur")); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any ideas how we can init the
SpanBuilder
with the already builtAttributes
map, to avoid this copy?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - I guess it requires an internal use API for that in the SDK. We can try it but it'll be more hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to clarify, did not mean for this pr, something (possibly) for the future..