diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java index 2628e9416cf..96c70a91de4 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java @@ -1,7 +1,5 @@ package datadog.trace.bootstrap.instrumentation.decorator; -import static datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver.hostName; - import datadog.context.Context; import datadog.context.ContextScope; import datadog.trace.api.Config; @@ -14,8 +12,6 @@ import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.Tags; import java.lang.reflect.Method; -import java.net.Inet4Address; -import java.net.Inet6Address; import java.net.InetAddress; import java.net.InetSocketAddress; import java.util.concurrent.ExecutionException; @@ -150,10 +146,7 @@ public ContextScope onError(final ContextScope scope, final Throwable throwable) public AgentSpan onPeerConnection( final AgentSpan span, final InetSocketAddress remoteConnection) { - if (remoteConnection != null) { - onPeerConnection(span, remoteConnection.getAddress(), !remoteConnection.isUnresolved()); - setPeerPort(span, remoteConnection.getPort()); - } + span.setTags(remoteConnection, PeerConnectionExtractor.INSTANCE); return span; } @@ -162,17 +155,7 @@ public AgentSpan onPeerConnection(final AgentSpan span, final InetAddress remote } public AgentSpan onPeerConnection(AgentSpan span, InetAddress remoteAddress, boolean resolved) { - if (remoteAddress != null) { - String ip = remoteAddress.getHostAddress(); - if (resolved && Config.get().isPeerHostNameEnabled()) { - span.setTag(Tags.PEER_HOSTNAME, hostName(remoteAddress, ip)); - } - if (remoteAddress instanceof Inet4Address) { - span.setTag(Tags.PEER_HOST_IPV4, ip); - } else if (remoteAddress instanceof Inet6Address) { - span.setTag(Tags.PEER_HOST_IPV6, ip); - } - } + PeerConnectionExtractor.setPeerAddress(span, remoteAddress, resolved); return span; } diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractor.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractor.java new file mode 100644 index 00000000000..f7767054fe6 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractor.java @@ -0,0 +1,57 @@ +package datadog.trace.bootstrap.instrumentation.decorator; + +import static datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver.hostName; + +import datadog.trace.api.Config; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.TagExtractor; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import java.net.Inet4Address; +import java.net.Inet6Address; +import java.net.InetAddress; +import java.net.InetSocketAddress; + +/** + * Named singleton {@link TagExtractor} for peer-connection tags (hostname / IPv4 / IPv6 / port) + * from a remote {@link InetSocketAddress}. + * + *

Promoted from an inline lambda in {@link BaseDecorator} to a named class so it can be: + * referenced by name at call sites ({@code span.setTags(addr, PeerConnectionExtractor.INSTANCE)}), + * composed with other extractors (the axis the Decorator inheritance chain lacked), and + * given a home for the pure statics and the {@code hostName} resolver cache it consults. + * Non-capturing and stateless — the single {@link #INSTANCE} is effectively a static function + * object, so a monomorphic call site inlines it away. + */ +public final class PeerConnectionExtractor implements TagExtractor { + public static final PeerConnectionExtractor INSTANCE = new PeerConnectionExtractor(); + + private static final int UNSET_PORT = 0; + + private PeerConnectionExtractor() {} + + @Override + public void extract(final InetSocketAddress remoteConnection, final AgentSpan span) { + // Non-null guaranteed by AgentSpan.setTags (the sole entry point); extractors skip + // null-handling. + setPeerAddress(span, remoteConnection.getAddress(), !remoteConnection.isUnresolved()); + final int port = remoteConnection.getPort(); + if (port > UNSET_PORT) { + span.setTag(Tags.PEER_PORT, port); + } + } + + static void setPeerAddress( + final AgentSpan span, final InetAddress remoteAddress, final boolean resolved) { + if (remoteAddress != null) { + String ip = remoteAddress.getHostAddress(); + if (resolved && Config.get().isPeerHostNameEnabled()) { + span.setTag(Tags.PEER_HOSTNAME, hostName(remoteAddress, ip)); + } + if (remoteAddress instanceof Inet4Address) { + span.setTag(Tags.PEER_HOST_IPV4, ip); + } else if (remoteAddress instanceof Inet6Address) { + span.setTag(Tags.PEER_HOST_IPV6, ip); + } + } + } +} diff --git a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy index 5b70cba2085..1a8b0c3d66b 100644 --- a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy +++ b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy @@ -46,29 +46,8 @@ class BaseDecoratorTest extends DDSpecification { 0 * _ } - def "test onPeerConnection"() { - when: - decorator.onPeerConnection(span, connection) - - then: - if (!connection.isUnresolved()) { - 1 * span.setTag(Tags.PEER_HOSTNAME, connection.hostName) - } - 1 * span.setTag(Tags.PEER_PORT, connection.port) - if (connection.address instanceof Inet4Address) { - 1 * span.setTag(Tags.PEER_HOST_IPV4, connection.address.hostAddress) - } - if (connection.address instanceof Inet6Address) { - 1 * span.setTag(Tags.PEER_HOST_IPV6, connection.address.hostAddress) - } - 0 * _ - - where: - connection | _ - new InetSocketAddress("localhost", 888) | _ - new InetSocketAddress("ipv6.google.com", 999) | _ - InetSocketAddress.createUnresolved("bad.address.local", 999) | _ - } + // "test onPeerConnection" migrated to PeerConnectionExtractorTest (JUnit 5): onPeerConnection now + // routes through AgentSpan.setTags, a default method a Spock mock won't dispatch to the extractor. def "test onError"() { when: diff --git a/dd-java-agent/agent-bootstrap/src/test/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractorTest.java b/dd-java-agent/agent-bootstrap/src/test/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractorTest.java new file mode 100644 index 00000000000..ac0ccd09255 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/test/java/datadog/trace/bootstrap/instrumentation/decorator/PeerConnectionExtractorTest.java @@ -0,0 +1,62 @@ +package datadog.trace.bootstrap.instrumentation.decorator; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.UnknownHostException; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link PeerConnectionExtractor#extract}. Covers the deterministic IP-family and + * port routing; the config/resolver-dependent {@code PEER_HOSTNAME} tag is intentionally not + * asserted (that behaviour belongs to the host-name resolver, and peer-hostname is on by default, + * so it would couple this test to reverse-DNS). Null source handling is the responsibility of + * {@code AgentSpan.setTags} (the extractor's sole entry point), not the extractor. + * + *

Literal IPs are used so no DNS lookup happens. + */ +class PeerConnectionExtractorTest { + + @Test + void ipv4AddressSetsIpv4TagAndPort() throws UnknownHostException { + final AgentSpan span = mock(AgentSpan.class); + final InetAddress addr = InetAddress.getByName("1.2.3.4"); // literal -> no DNS + + PeerConnectionExtractor.INSTANCE.extract(new InetSocketAddress(addr, 8080), span); + + verify(span).setTag(Tags.PEER_HOST_IPV4, addr.getHostAddress()); + verify(span).setTag(Tags.PEER_PORT, 8080); + verify(span, never()).setTag(eq(Tags.PEER_HOST_IPV6), anyString()); + } + + @Test + void ipv6AddressSetsIpv6TagAndPort() throws UnknownHostException { + final AgentSpan span = mock(AgentSpan.class); + final InetAddress addr = InetAddress.getByName("2001:db8::1"); // literal -> no DNS + + PeerConnectionExtractor.INSTANCE.extract(new InetSocketAddress(addr, 9090), span); + + verify(span).setTag(Tags.PEER_HOST_IPV6, addr.getHostAddress()); + verify(span).setTag(Tags.PEER_PORT, 9090); + verify(span, never()).setTag(eq(Tags.PEER_HOST_IPV4), anyString()); + } + + @Test + void unresolvedAddressSetsOnlyPort() { + final AgentSpan span = mock(AgentSpan.class); + + PeerConnectionExtractor.INSTANCE.extract( + InetSocketAddress.createUnresolved("no.dns.local", 999), span); + + verify(span).setTag(Tags.PEER_PORT, 999); + verify(span, never()).setTag(eq(Tags.PEER_HOST_IPV4), anyString()); + verify(span, never()).setTag(eq(Tags.PEER_HOST_IPV6), anyString()); + } +} diff --git a/internal-api/src/jmh/java/datadog/trace/api/TagProjectionBenchmark.java b/internal-api/src/jmh/java/datadog/trace/api/TagProjectionBenchmark.java new file mode 100644 index 00000000000..b37eb514b4f --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/api/TagProjectionBenchmark.java @@ -0,0 +1,373 @@ +package datadog.trace.api; + +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Models projecting a typed source's fields onto a span three ways — the decorator template-method + * pattern vs. the {@code TagExtractor} / {@code TagContributor} shape — to show what compiles away + * and what doesn't. Uses synthetic structural equivalents (a light DCE-safe {@link Sink} stands in + * for the span) so it isolates *dispatch* cost, not span/tag-storage cost; the real interfaces + * target {@code AgentSpan}, but the call-site shape is what this measures. + * + *

{@code mode}: + * + *

+ * + *

Run with {@code -prof gc} (captures should stay flat) and {@code -XX:+PrintInlining} to + * confirm the mechanism: decorator template calls megamorphic/not-inlined under mega; + * extractor/contributor inlined under mono. Throughput is the headline; per your methodology, + * multi-thread reveals alloc pressure and 3+ forks expose bimodal JIT/devirt behavior. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 5, time = 2) +@Measurement(iterations = 5, time = 2) +@Fork(3) +@Threads(8) +public class TagProjectionBenchmark { + static final int TYPES = 8; + + @Param({"mono", "mega"}) + String mode; + + /** Light DCE-safe stand-in for the span — identical trivial work per set across all arms. */ + static final class Sink { + long acc; + + void set(String key, Object value) { + this.acc += key.length() + (value == null ? 0 : value.hashCode()); + } + } + + /** The typed source whose fields are projected (a stand-in for DbInfo). */ + static final class DbPojo { + final String dbType, dbUser, dbInstance, dbHostname; + + DbPojo(String t, String u, String i, String h) { + this.dbType = t; + this.dbUser = u; + this.dbInstance = i; + this.dbHostname = h; + } + } + + // ---- (1) decorator template-method pattern: shared base onConnection() calls abstract hooks + // ---- + abstract static class TemplateDecorator { + abstract String dbType(DbPojo p); + + abstract String dbUser(DbPojo p); + + abstract String dbInstance(DbPojo p); + + abstract String dbHostname(DbPojo p); + + // shared base: calls the abstract template methods -> megamorphic when many subclasses loaded + final void onConnection(Sink sink, DbPojo p) { + String type = dbType(p); + if (type != null) sink.set("db.type", type); + String user = dbUser(p); + if (user != null) sink.set("db.user", user); + String instance = dbInstance(p); + if (instance != null) sink.set("db.instance", instance); + String host = dbHostname(p); + if (host != null) sink.set("peer.hostname", host); + } + } + + // ---- (2) extractor shape: stateless lambda-style, reads source -> sets directly ---- + interface Extractor { + void extract(DbPojo p, Sink sink); + } + + // ---- (3) contributor shape: the source projects itself ---- + interface Contributor { + void addTo(Sink sink); + } + + private DbPojo[] pojos; + private TemplateDecorator[] decorators; + private Extractor[] extractors; + private Contributor[] contributors; + private Sink sink; + private int idx; + + @Setup(Level.Trial) + public void setup() { + this.sink = new Sink(); + this.pojos = new DbPojo[TYPES]; + this.decorators = new TemplateDecorator[TYPES]; + this.extractors = new Extractor[TYPES]; + this.contributors = new Contributor[TYPES]; + for (int i = 0; i < TYPES; i++) { + DbPojo pojo = new DbPojo("h2", "sa", "db" + i, "localhost"); + this.pojos[i] = pojo; + this.decorators[i] = newDecorator(i); + this.extractors[i] = newExtractor(i); + this.contributors[i] = newContributor(pojo, i); + } + } + + // distinct concrete types per index so 'mega' has TYPES loaded implementations to megamorphize + private static TemplateDecorator newDecorator(int i) { + switch (i) { + case 0: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 1: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 2: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 3: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 4: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 5: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + case 6: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + default: + return new TemplateDecorator() { + String dbType(DbPojo p) { + return p.dbType; + } + + String dbUser(DbPojo p) { + return p.dbUser; + } + + String dbInstance(DbPojo p) { + return p.dbInstance; + } + + String dbHostname(DbPojo p) { + return p.dbHostname; + } + }; + } + } + + // distinct Extractor lambda expressions -> distinct synthetic types (megamorphic when rotated) + private static Extractor newExtractor(int i) { + switch (i) { + case 0: + return TagProjectionBenchmark::extract; + case 1: + return (p, s) -> extract(p, s); + case 2: + return (DbPojo p, Sink s) -> extract(p, s); + case 3: + return (p, s) -> { + extract(p, s); + }; + case 4: + return (final DbPojo p, final Sink s) -> extract(p, s); + case 5: + return (p, s) -> extract(p, s); + case 6: + return (DbPojo p, Sink s) -> { + extract(p, s); + }; + default: + return (p, s) -> extract(p, s); + } + } + + // the actual projection, shared so all extractor variants do identical work + private static void extract(DbPojo p, Sink sink) { + if (p.dbType != null) sink.set("db.type", p.dbType); + if (p.dbUser != null) sink.set("db.user", p.dbUser); + if (p.dbInstance != null) sink.set("db.instance", p.dbInstance); + if (p.dbHostname != null) sink.set("peer.hostname", p.dbHostname); + } + + private static Contributor newContributor(DbPojo p, int i) { + switch (i) { + case 0: + return s -> extract(p, s); + case 1: + return (Sink s) -> extract(p, s); + case 2: + return s -> { + extract(p, s); + }; + case 3: + return (final Sink s) -> extract(p, s); + case 4: + return s -> extract(p, s); + case 5: + return (Sink s) -> { + extract(p, s); + }; + case 6: + return s -> extract(p, s); + default: + return (Sink s) -> extract(p, s); + } + } + + // mono: always index 0 (one type at the site). mega: rotate -> the site sees all TYPES. + private int next() { + if ("mono".equals(this.mode)) return 0; + int i = this.idx + 1; + if (i >= TYPES) i = 0; + this.idx = i; + return i; + } + + @Benchmark + public long decorator() { + int i = next(); + this.decorators[i].onConnection(this.sink, this.pojos[i]); + return this.sink.acc; + } + + @Benchmark + public long extractor() { + int i = next(); + this.extractors[i].extract(this.pojos[i], this.sink); + return this.sink.acc; + } + + @Benchmark + public long contributor() { + int i = next(); + this.contributors[i].addTo(this.sink); + return this.sink.acc; + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index 0eb67f3ad52..46257f93aeb 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -88,6 +88,20 @@ default boolean isValid() { AgentSpan setAllTags(Map map); + /** + * Sets tags on this span by applying a {@link TagExtractor} to a foreign source — the span-first, + * product-dev-facing form of {@code extractor.extract(source, this)} (matches {@code + * setTag}/{@code setAllTags}). The extra indirection inlines away at a monomorphic call site. + * This is also the seam where the whole extraction can be coarse-locked (one critical section) — + * an implementer may override to do so. + */ + default AgentSpan setTags(final T source, final TagExtractor extractor) { + if (source != null) { + extractor.extract(source, this); + } + return this; + } + @Override AgentSpan setTag(String key, Number value); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java new file mode 100644 index 00000000000..e3f7877e27b --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java @@ -0,0 +1,23 @@ +package datadog.trace.bootstrap.instrumentation.api; + +/** + * An object that projects its own typed state onto a span — the inverse of a builder that takes + * external {@code setTag} calls. Implemented by typed POJOs we own whose fields map to known tags + * (e.g. {@code DbInfo}, git metadata, extracted context). {@code addTo} is a flat sweep of {@code + * span.setTag(...)}; once the id-arm lands it becomes {@code setTag(KnownTagIds.X, field)}, so it + * compiles to field-loads + positional stores with no {@code keyOf}. + * + *

This is an authoring aid meant to compile to ~zero — the opposite of a Decorator. Apply it at + * a CONCRETE-type, monomorphic call site (the integration's own advice) so {@code addTo} + * devirtualizes and inlines. Do not route many contributors through one shared {@code + * List} sink — that re-megamorphizes and recreates the decorator problem. Prefer + * stateless implementations. + * + *

Targets {@link AgentSpan} for now (so it can also drive span-level state — resource name, + * error, measured — that is not yet expressed as tags). As those span fields migrate into the tag + * model, this surface narrows toward a future {@code addTo(TagMap)}. See {@link TagExtractor} for + * the extrinsic counterpart (foreign objects we do not own). + */ +public interface TagContributor { + void addTo(AgentSpan span); +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java new file mode 100644 index 00000000000..4881308a231 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java @@ -0,0 +1,30 @@ +package datadog.trace.bootstrap.instrumentation.api; + +/** + * Extracts tags from a foreign object we do not own (a framework {@code Connection}, request, + * response, etc.) onto a span — the extrinsic counterpart to {@link TagContributor}. This is the + * irreducible "reach into version-specific framework objects" that cannot be expressed as data; it + * stays imperative, but contained in a narrow, single-purpose place. + * + *

Two usage modes, chosen by the source's lifecycle vs. the span: + * + *

+ * + *

This is an authoring aid meant to compile to ~zero — the opposite of a Decorator. Intended as + * a {@code static final}, non-capturing lambda invoked from the integration's own advice: a + * monomorphic call site the JIT devirtualizes and inlines (strictly better than dispatching through + * the decorator hierarchy). Never route many extractors through one shared site. Targets {@link + * AgentSpan} so it can also drive span-level state (resource name, error, status) during the + * transition; that surface narrows as those fields migrate into the tag model. + * + * @param the foreign source type to extract from + */ +@FunctionalInterface +public interface TagExtractor { + void extract(T source, AgentSpan span); +}