diff --git a/.agents/skills/perf-review/references/checks.md b/.agents/skills/perf-review/references/checks.md index 3de39dec2a0..59b0b551036 100644 --- a/.agents/skills/perf-review/references/checks.md +++ b/.agents/skills/perf-review/references/checks.md @@ -34,6 +34,7 @@ Fixed-signature, mechanically checkable: - boxing in specific hot APIs - using a string-API where an id-API exists on a hot decorator - the existing convention rules (e.g. don't extract one-shot instrumentation methods to constants) +- synthetic accessors (`access$*` methods, synthetic constructors) in a nest marked `@NoSyntheticAccessors` (`datadog.perfcontract.NoSyntheticAccessors`) — **already a real check**: a test per annotated class calls `datadog.trace.test.util.SyntheticAccessors.assertNone`, so don't spend AI review on it - *(grows as patterns prove mechanically checkable — migrate them off the AI as they stabilize; the `@NoEscape` and `@StaticLifetime` field-storage checks above belong here once their checkers land)* ## Java addendum (JVM-specific — mechanism authored with JIT-developer authority; **calibrate production-priority against your own escalation history**) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index 8f52b49052c..921e01b4767 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -33,6 +33,8 @@ import datadog.metrics.api.Monitoring; import datadog.metrics.api.Recording; import datadog.metrics.api.statsd.StatsDClient; +import datadog.perfcontract.NoSyntheticAccessors; +import datadog.perfcontract.SuppressPerfContract; import datadog.trace.api.ClassloaderConfigurationOverrides; import datadog.trace.api.Config; import datadog.trace.api.DDSpanId; @@ -144,6 +146,7 @@ * datadog.trace.api.Tracer and TracerAPI, it coordinates many functions necessary creating, * reporting, and propagating traces */ +@NoSyntheticAccessors public class CoreTracer implements AgentTracer.TracerAPI, TracerFlare.Reporter { private static final Logger log = LoggerFactory.getLogger(CoreTracer.class); @@ -222,8 +225,14 @@ private static final class LocalRootSpanTags { private final boolean apmTracingEnabled; /** A set of tags that are added to every span */ + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "read by ConfigSnapshot, once per config snapshot") private final TagMap defaultSpanTags; + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "read by ConfigSnapshot, once per config snapshot") private final boolean defaultSpanTagsNeedsIntercept; /** number of spans in a pending trace before they get flushed */ @@ -255,6 +264,9 @@ private static final class LocalRootSpanTags { * Span tag interceptors. This Map is only ever added to during initialization, so it doesn't need * to be concurrent. */ + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "read by ConfigSnapshot, on a config reload that changes tracing tags") private final TagInterceptor tagInterceptor; private final TraceInterceptors interceptors = new TraceInterceptors(); @@ -649,6 +661,9 @@ private CoreTracer( } // These field names must be stable to ensure the builder api is stable. + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "called once, by CoreTracerBuilder.build()") private CoreTracer( final Config config, final String serviceName, @@ -1672,6 +1687,9 @@ Recording writeTimer() { return traceWriteTimer.start(); } + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "called once, while building the tracer") private static Map invertMap(Map map) { Map inverted = new HashMap<>(map.size()); for (Map.Entry entry : map.entrySet()) { @@ -1680,6 +1698,306 @@ private static Map invertMap(Map map) { return Collections.unmodifiableMap(inverted); } + /** + * Build the SpanContext, if the actual span has a parent, the following attributes must be + * propagated: - ServiceName - Baggage - Trace (a list of all spans related) - SpanType + * + * @return the context + */ + DDSpanContext buildSpanContext( + long spanId, + String serviceName, + CharSequence operationName, + String resourceName, + AgentSpanContext resolvedParentSpanContext, + boolean errorFlag, + CharSequence spanType, + TagMap.Ledger tagLedger, + SpanPrototype spanPrototype, + List links, + Object builderRequestContextDataAppSec, + Object builderRequestContextDataIast, + Object builderCiVisibilityContextData) { + DDTraceId traceId; + long parentSpanId; + final Map baggage; + final Baggage w3cBaggage; + final TraceCollector parentTraceCollector; + final int samplingPriority; + final CharSequence origin; + final TagMap coreTags; + final boolean coreTagsNeedsIntercept; + final TagMap rootSpanTags; + final boolean rootSpanTagsNeedsIntercept; + final DDSpanContext context; + Object requestContextDataAppSec; + Object requestContextDataIast; + Object ciVisibilityContextData; + final PathwayContext pathwayContext; + final PropagationTags propagationTags; + + if (spanId == CoreSpanBuilder.AUTO_ASSIGN_SPAN_ID) { + spanId = this.idGenerationStrategy.generateSpanId(); + } + + String parentServiceName = null; + CharSequence serviceNameSource = MANUAL; + // Propagate internal trace. + // Note: if we are not in the context of distributed tracing, and we are starting the first + // root span, parentSpanContext will be null at this point. + if (resolvedParentSpanContext instanceof DDSpanContext) { + final DDSpanContext ddsc = (DDSpanContext) resolvedParentSpanContext; + traceId = ddsc.getTraceId(); + parentSpanId = ddsc.getSpanId(); + baggage = ddsc.getBaggageItems(); + w3cBaggage = null; + parentTraceCollector = ddsc.getTraceCollector(); + samplingPriority = PrioritySampling.UNSET; + origin = null; + coreTags = null; + coreTagsNeedsIntercept = false; + rootSpanTags = null; + rootSpanTagsNeedsIntercept = false; + parentServiceName = ddsc.getServiceName(); + serviceNameSource = ddsc.getServiceNameSource(); + if (serviceName == null) { + serviceName = parentServiceName; + } + RequestContext requestContext = + ((DDSpanContext) resolvedParentSpanContext).getRequestContext(); + if (requestContext != null) { + requestContextDataAppSec = requestContext.getData(RequestContextSlot.APPSEC); + requestContextDataIast = requestContext.getData(RequestContextSlot.IAST); + ciVisibilityContextData = requestContext.getData(RequestContextSlot.CI_VISIBILITY); + } else { + requestContextDataAppSec = null; + requestContextDataIast = null; + ciVisibilityContextData = null; + } + propagationTags = this.propagationTagsFactory.empty(); + } else { + long endToEndStartTime; + + if (resolvedParentSpanContext instanceof ExtractedContext) { + // Propagate external trace + final ExtractedContext extractedContext = (ExtractedContext) resolvedParentSpanContext; + traceId = extractedContext.getTraceId(); + parentSpanId = extractedContext.getSpanId(); + samplingPriority = extractedContext.getSamplingPriority(); + endToEndStartTime = extractedContext.getEndToEndStartTime(); + propagationTags = extractedContext.getPropagationTags(); + } else if (resolvedParentSpanContext != null) { + traceId = + resolvedParentSpanContext.getTraceId() == DDTraceId.ZERO + ? this.idGenerationStrategy.generateTraceId() + : resolvedParentSpanContext.getTraceId(); + parentSpanId = resolvedParentSpanContext.getSpanId(); + samplingPriority = resolvedParentSpanContext.getSamplingPriority(); + endToEndStartTime = 0; + propagationTags = this.propagationTagsFactory.empty(); + } else { + // Start a new trace + traceId = this.idGenerationStrategy.generateTraceId(); + parentSpanId = DDSpanId.ZERO; + samplingPriority = PrioritySampling.UNSET; + endToEndStartTime = 0; + propagationTags = this.propagationTagsFactory.empty(); + } + + ConfigSnapshot traceConfig; + + // Get header tags and set origin whether propagating or not. + if (resolvedParentSpanContext instanceof TagContext) { + TagContext tc = (TagContext) resolvedParentSpanContext; + traceConfig = (ConfigSnapshot) tc.getTraceConfig(); + coreTags = tc.getTags(); + coreTagsNeedsIntercept = true; // maybe intercept isn't needed? + origin = tc.getOrigin(); + baggage = tc.getBaggage(); + w3cBaggage = tc.getW3CBaggage(); + requestContextDataAppSec = tc.getRequestContextDataAppSec(); + requestContextDataIast = tc.getRequestContextDataIast(); + ciVisibilityContextData = tc.getCiVisibilityContextData(); + } else { + traceConfig = null; + coreTags = null; + coreTagsNeedsIntercept = false; + origin = null; + baggage = null; + w3cBaggage = null; + requestContextDataAppSec = null; + requestContextDataIast = null; + ciVisibilityContextData = null; + } + + LocalRootSpanTags localRootSpanTags = this.localRootSpanTags; + rootSpanTags = localRootSpanTags.tags; + rootSpanTagsNeedsIntercept = localRootSpanTags.needsIntercept; + + parentTraceCollector = this.createTraceCollector(traceId, traceConfig); + + if (endToEndStartTime > 0) { + parentTraceCollector.beginEndToEnd(endToEndStartTime); + } + } + + ConfigSnapshot traceConfig = parentTraceCollector.getTraceConfig(); + + // Use parent pathwayContext if present and started + pathwayContext = + resolvedParentSpanContext != null + && resolvedParentSpanContext.getPathwayContext() != null + && resolvedParentSpanContext.getPathwayContext().isStarted() + ? resolvedParentSpanContext.getPathwayContext() + : this.dataStreamsMonitoring.newPathwayContext(); + + // when removing fake services the best upward service name to pick is the local root one + // since a split by tag (i.e. servlet context) might have happened on it. + if (!this.allowInferredServices) { + final DDSpan rootSpan = parentTraceCollector.getRootSpan(); + if (rootSpan != null) { + // An inferred proxy represents the gateway, not the application service. + // Preserve the service and source already resolved for spans beneath it. + // Avoid the synchronized tag lookup when inferred proxies are disabled. + if (!this.initialConfig.isInferredProxyPropagationEnabled() + || rootSpan.getTag("_dd.inferred_span") == null) { + serviceName = rootSpan.getServiceName(); + serviceNameSource = rootSpan.getServiceNameSource(); + } + } else { + serviceName = null; + } + } + if (serviceName == null) { + final Pair serviceNameAndSource = + traceConfig.getPreferredServiceNameAndSource(); + ; + if (serviceNameAndSource != null && serviceNameAndSource.hasLeft()) { + serviceName = serviceNameAndSource.getLeft(); + serviceNameSource = serviceNameAndSource.getRight(); + } + } + Map contextualTags = null; + if (parentServiceName == null) { + // only fetch this on local root spans + final ClassloaderConfigurationOverrides.ContextualInfo contextualInfo = + ClassloaderConfigurationOverrides.maybeGetContextualInfo(); + if (contextualInfo != null) { + // in this case we have a local root without service name. + // We can try to see if we can find one from the thread context classloader + if (serviceName == null) { + serviceName = contextualInfo.getServiceName(); + if (serviceName != null) { + serviceNameSource = contextualInfo.getServiceNameSource(); + } + } + contextualTags = contextualInfo.getTags(); + } + } + if (serviceName == null) { + // it could be on the initial snapshot but may be overridden to null and service name + // cannot be null + serviceName = this.serviceName; + // do not mark as manual when the service name is coming from the tracer default + serviceNameSource = null; + } + + if (operationName == null) { + operationName = resourceName; + } + + final TagMap mergedTracerTags = traceConfig.mergedTracerTags; + boolean mergedTracerTagsNeedsIntercept = traceConfig.mergedTracerTagsNeedsIntercept; + + final int tagsSize = + mergedTracerTags.size() + + (null == tagLedger ? 0 : tagLedger.estimateSize()) + + (null == coreTags ? 0 : coreTags.size()) + + (null == rootSpanTags ? 0 : rootSpanTags.size()) + + (null == contextualTags ? 0 : contextualTags.size()); + + if (builderRequestContextDataAppSec != null) { + requestContextDataAppSec = builderRequestContextDataAppSec; + } + if (builderCiVisibilityContextData != null) { + ciVisibilityContextData = builderCiVisibilityContextData; + } + if (builderRequestContextDataIast != null) { + requestContextDataIast = builderRequestContextDataIast; + } + + // some attributes are inherited from the parent + context = + new DDSpanContext( + traceId, + spanId, + parentSpanId, + parentServiceName, + serviceNameSource, + serviceName, + operationName, + resourceName, + samplingPriority, + origin, + baggage, + w3cBaggage, + errorFlag, + spanType, + tagsSize, + parentTraceCollector, + requestContextDataAppSec, + requestContextDataIast, + ciVisibilityContextData, + pathwayContext, + this.disableSamplingMechanismValidation, + propagationTags, + this.profilingContextIntegration, + this.injectBaggageAsTags, + this.injectLinksAsTags, + mergedTracerTagsNeedsIntercept ? null : mergedTracerTags); + + // By setting the tags on the context we apply decorators to any tags that have been set via + // the builder. The `mergedTracerTags` are always applied first (the precedence floor: + // everything overrides them). The remaining contributors are applied last-wins; with + // `builderTagsPrecedence` enabled, `tagLedger` (the explicit builder tags) moves to last so + // it wins collisions instead of being overridden by `coreTags`/`rootSpanTags`/ + // `contextualTags` -- see the PR description for the historical context on this ordering. + // + // mergedTracerTags is trace-level shared state and the precedence floor (everything below + // overrides it). When it carries no interceptable tags it is attached as a read-through + // PARENT at construction (shared by reference, no per-span copy). When it does need + // interception, copy its entries in (the interceptor's per-span side-effects can't be + // shared by reference). + if (mergedTracerTagsNeedsIntercept) { + context.setAllTags(mergedTracerTags, true); + } + if (spanPrototype != SpanPrototype.NONE) { + // Seed the prototype's constant tags + integration name as fallback defaults (span type was + // already seeded onto the builder). apply never clobbers, so tags set below still win, and + // this is the same seam decorator afterStart uses. + context.apply(spanPrototype); + } + if (this.builderTagsPrecedence) { + context.setAllTags(coreTags, coreTagsNeedsIntercept); + context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); + context.setAllTags(contextualTags); + context.setAllTags(tagLedger); + } else { + context.setAllTags(tagLedger); + context.setAllTags(coreTags, coreTagsNeedsIntercept); + context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); + context.setAllTags(contextualTags); + } + // Version is added later by the postProcessor (InternalTagsAdder), only if not already set + // during the request. Config version is kept out of the trace-level bundle (see + // withTracerTags), so this removal now only wipes a version set via the span builder — + // keeping the existing semantics where a builder-set version is replaced by the config + // version. Under read-through this is a cheap local removal (version isn't in the parent, + // so no tombstone). + context.removeTag(Tags.VERSION); + return context; + } + /** Spans are built using this builder */ public abstract static class CoreSpanBuilder implements AgentTracer.SpanBuilder { protected static final boolean USE_SCOPE = false; @@ -1744,8 +2062,7 @@ protected static final DDSpan buildSpan( instrumentationName, timestampMicro, links, - buildSpanContext( - tracer, + tracer.buildSpanContext( spanId, serviceName, operationName, @@ -2098,307 +2415,6 @@ public final CoreSpanBuilder withSpanId(final long spanId) { this.spanId = spanId; return this; } - - /** - * Build the SpanContext, if the actual span has a parent, the following attributes must be - * propagated: - ServiceName - Baggage - Trace (a list of all spans related) - SpanType - * - * @return the context - */ - protected static final DDSpanContext buildSpanContext( - final CoreTracer tracer, - long spanId, - String serviceName, - CharSequence operationName, - String resourceName, - AgentSpanContext resolvedParentSpanContext, - boolean errorFlag, - CharSequence spanType, - TagMap.Ledger tagLedger, - SpanPrototype spanPrototype, - List links, - Object builderRequestContextDataAppSec, - Object builderRequestContextDataIast, - Object builderCiVisibilityContextData) { - DDTraceId traceId; - long parentSpanId; - final Map baggage; - final Baggage w3cBaggage; - final TraceCollector parentTraceCollector; - final int samplingPriority; - final CharSequence origin; - final TagMap coreTags; - final boolean coreTagsNeedsIntercept; - final TagMap rootSpanTags; - final boolean rootSpanTagsNeedsIntercept; - final DDSpanContext context; - Object requestContextDataAppSec; - Object requestContextDataIast; - Object ciVisibilityContextData; - final PathwayContext pathwayContext; - final PropagationTags propagationTags; - - if (spanId == AUTO_ASSIGN_SPAN_ID) { - spanId = tracer.idGenerationStrategy.generateSpanId(); - } - - String parentServiceName = null; - CharSequence serviceNameSource = MANUAL; - // Propagate internal trace. - // Note: if we are not in the context of distributed tracing, and we are starting the first - // root span, parentSpanContext will be null at this point. - if (resolvedParentSpanContext instanceof DDSpanContext) { - final DDSpanContext ddsc = (DDSpanContext) resolvedParentSpanContext; - traceId = ddsc.getTraceId(); - parentSpanId = ddsc.getSpanId(); - baggage = ddsc.getBaggageItems(); - w3cBaggage = null; - parentTraceCollector = ddsc.getTraceCollector(); - samplingPriority = PrioritySampling.UNSET; - origin = null; - coreTags = null; - coreTagsNeedsIntercept = false; - rootSpanTags = null; - rootSpanTagsNeedsIntercept = false; - parentServiceName = ddsc.getServiceName(); - serviceNameSource = ddsc.getServiceNameSource(); - if (serviceName == null) { - serviceName = parentServiceName; - } - RequestContext requestContext = - ((DDSpanContext) resolvedParentSpanContext).getRequestContext(); - if (requestContext != null) { - requestContextDataAppSec = requestContext.getData(RequestContextSlot.APPSEC); - requestContextDataIast = requestContext.getData(RequestContextSlot.IAST); - ciVisibilityContextData = requestContext.getData(RequestContextSlot.CI_VISIBILITY); - } else { - requestContextDataAppSec = null; - requestContextDataIast = null; - ciVisibilityContextData = null; - } - propagationTags = tracer.propagationTagsFactory.empty(); - } else { - long endToEndStartTime; - - if (resolvedParentSpanContext instanceof ExtractedContext) { - // Propagate external trace - final ExtractedContext extractedContext = (ExtractedContext) resolvedParentSpanContext; - traceId = extractedContext.getTraceId(); - parentSpanId = extractedContext.getSpanId(); - samplingPriority = extractedContext.getSamplingPriority(); - endToEndStartTime = extractedContext.getEndToEndStartTime(); - propagationTags = extractedContext.getPropagationTags(); - } else if (resolvedParentSpanContext != null) { - traceId = - resolvedParentSpanContext.getTraceId() == DDTraceId.ZERO - ? tracer.idGenerationStrategy.generateTraceId() - : resolvedParentSpanContext.getTraceId(); - parentSpanId = resolvedParentSpanContext.getSpanId(); - samplingPriority = resolvedParentSpanContext.getSamplingPriority(); - endToEndStartTime = 0; - propagationTags = tracer.propagationTagsFactory.empty(); - } else { - // Start a new trace - traceId = tracer.idGenerationStrategy.generateTraceId(); - parentSpanId = DDSpanId.ZERO; - samplingPriority = PrioritySampling.UNSET; - endToEndStartTime = 0; - propagationTags = tracer.propagationTagsFactory.empty(); - } - - ConfigSnapshot traceConfig; - - // Get header tags and set origin whether propagating or not. - if (resolvedParentSpanContext instanceof TagContext) { - TagContext tc = (TagContext) resolvedParentSpanContext; - traceConfig = (ConfigSnapshot) tc.getTraceConfig(); - coreTags = tc.getTags(); - coreTagsNeedsIntercept = true; // maybe intercept isn't needed? - origin = tc.getOrigin(); - baggage = tc.getBaggage(); - w3cBaggage = tc.getW3CBaggage(); - requestContextDataAppSec = tc.getRequestContextDataAppSec(); - requestContextDataIast = tc.getRequestContextDataIast(); - ciVisibilityContextData = tc.getCiVisibilityContextData(); - } else { - traceConfig = null; - coreTags = null; - coreTagsNeedsIntercept = false; - origin = null; - baggage = null; - w3cBaggage = null; - requestContextDataAppSec = null; - requestContextDataIast = null; - ciVisibilityContextData = null; - } - - LocalRootSpanTags localRootSpanTags = tracer.localRootSpanTags; - rootSpanTags = localRootSpanTags.tags; - rootSpanTagsNeedsIntercept = localRootSpanTags.needsIntercept; - - parentTraceCollector = tracer.createTraceCollector(traceId, traceConfig); - - if (endToEndStartTime > 0) { - parentTraceCollector.beginEndToEnd(endToEndStartTime); - } - } - - ConfigSnapshot traceConfig = parentTraceCollector.getTraceConfig(); - - // Use parent pathwayContext if present and started - pathwayContext = - resolvedParentSpanContext != null - && resolvedParentSpanContext.getPathwayContext() != null - && resolvedParentSpanContext.getPathwayContext().isStarted() - ? resolvedParentSpanContext.getPathwayContext() - : tracer.dataStreamsMonitoring.newPathwayContext(); - - // when removing fake services the best upward service name to pick is the local root one - // since a split by tag (i.e. servlet context) might have happened on it. - if (!tracer.allowInferredServices) { - final DDSpan rootSpan = parentTraceCollector.getRootSpan(); - if (rootSpan != null) { - // An inferred proxy represents the gateway, not the application service. - // Preserve the service and source already resolved for spans beneath it. - // Avoid the synchronized tag lookup when inferred proxies are disabled. - if (!tracer.initialConfig.isInferredProxyPropagationEnabled() - || rootSpan.getTag("_dd.inferred_span") == null) { - serviceName = rootSpan.getServiceName(); - serviceNameSource = rootSpan.getServiceNameSource(); - } - } else { - serviceName = null; - } - } - if (serviceName == null) { - final Pair serviceNameAndSource = - traceConfig.getPreferredServiceNameAndSource(); - ; - if (serviceNameAndSource != null && serviceNameAndSource.hasLeft()) { - serviceName = serviceNameAndSource.getLeft(); - serviceNameSource = serviceNameAndSource.getRight(); - } - } - Map contextualTags = null; - if (parentServiceName == null) { - // only fetch this on local root spans - final ClassloaderConfigurationOverrides.ContextualInfo contextualInfo = - ClassloaderConfigurationOverrides.maybeGetContextualInfo(); - if (contextualInfo != null) { - // in this case we have a local root without service name. - // We can try to see if we can find one from the thread context classloader - if (serviceName == null) { - serviceName = contextualInfo.getServiceName(); - if (serviceName != null) { - serviceNameSource = contextualInfo.getServiceNameSource(); - } - } - contextualTags = contextualInfo.getTags(); - } - } - if (serviceName == null) { - // it could be on the initial snapshot but may be overridden to null and service name - // cannot be null - serviceName = tracer.serviceName; - // do not mark as manual when the service name is coming from the tracer default - serviceNameSource = null; - } - - if (operationName == null) { - operationName = resourceName; - } - - final TagMap mergedTracerTags = traceConfig.mergedTracerTags; - boolean mergedTracerTagsNeedsIntercept = traceConfig.mergedTracerTagsNeedsIntercept; - - final int tagsSize = - mergedTracerTags.size() - + (null == tagLedger ? 0 : tagLedger.estimateSize()) - + (null == coreTags ? 0 : coreTags.size()) - + (null == rootSpanTags ? 0 : rootSpanTags.size()) - + (null == contextualTags ? 0 : contextualTags.size()); - - if (builderRequestContextDataAppSec != null) { - requestContextDataAppSec = builderRequestContextDataAppSec; - } - if (builderCiVisibilityContextData != null) { - ciVisibilityContextData = builderCiVisibilityContextData; - } - if (builderRequestContextDataIast != null) { - requestContextDataIast = builderRequestContextDataIast; - } - - // some attributes are inherited from the parent - context = - new DDSpanContext( - traceId, - spanId, - parentSpanId, - parentServiceName, - serviceNameSource, - serviceName, - operationName, - resourceName, - samplingPriority, - origin, - baggage, - w3cBaggage, - errorFlag, - spanType, - tagsSize, - parentTraceCollector, - requestContextDataAppSec, - requestContextDataIast, - ciVisibilityContextData, - pathwayContext, - tracer.disableSamplingMechanismValidation, - propagationTags, - tracer.profilingContextIntegration, - tracer.injectBaggageAsTags, - tracer.injectLinksAsTags, - mergedTracerTagsNeedsIntercept ? null : mergedTracerTags); - - // By setting the tags on the context we apply decorators to any tags that have been set via - // the builder. The `mergedTracerTags` are always applied first (the precedence floor: - // everything overrides them). The remaining contributors are applied last-wins; with - // `builderTagsPrecedence` enabled, `tagLedger` (the explicit builder tags) moves to last so - // it wins collisions instead of being overridden by `coreTags`/`rootSpanTags`/ - // `contextualTags` -- see the PR description for the historical context on this ordering. - // - // mergedTracerTags is trace-level shared state and the precedence floor (everything below - // overrides it). When it carries no interceptable tags it is attached as a read-through - // PARENT at construction (shared by reference, no per-span copy). When it does need - // interception, copy its entries in (the interceptor's per-span side-effects can't be - // shared by reference). - if (mergedTracerTagsNeedsIntercept) { - context.setAllTags(mergedTracerTags, true); - } - if (spanPrototype != SpanPrototype.NONE) { - // Seed the prototype's constant tags + integration name as fallback defaults (span type was - // already seeded onto the builder). apply never clobbers, so tags set below still win, and - // this is the same seam decorator afterStart uses. - context.apply(spanPrototype); - } - if (tracer.builderTagsPrecedence) { - context.setAllTags(coreTags, coreTagsNeedsIntercept); - context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); - context.setAllTags(contextualTags); - context.setAllTags(tagLedger); - } else { - context.setAllTags(tagLedger); - context.setAllTags(coreTags, coreTagsNeedsIntercept); - context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); - context.setAllTags(contextualTags); - } - // Version is added later by the postProcessor (InternalTagsAdder), only if not already set - // during the request. Config version is kept out of the trace-level bundle (see - // withTracerTags), so this removal now only wipes a version set via the span builder — - // keeping the existing semantics where a builder-set version is replaced by the config - // version. Under read-through this is a cheap local removal (version isn't in the parent, - // so no tombstone). - context.removeTag(Tags.VERSION); - return context; - } } /** CoreSpanBuilder that can be used to produce multiple spans */ @@ -2573,6 +2589,9 @@ public synchronized TraceInterceptor add(TraceInterceptor newInterceptor) { private static class ShutdownHook extends Thread { private final WeakReference reference; + @SuppressPerfContract( + value = NoSyntheticAccessors.class, + reason = "constructed once, when the tracer registers its shutdown hook") private ShutdownHook(final CoreTracer tracer) { super(AGENT_THREAD_GROUP, "dd-tracer-shutdown-hook"); reference = new WeakReference<>(tracer); diff --git a/dd-trace-core/src/test/java/datadog/trace/core/CoreTracerSyntheticAccessorsTest.java b/dd-trace-core/src/test/java/datadog/trace/core/CoreTracerSyntheticAccessorsTest.java new file mode 100644 index 00000000000..355d9536ef7 --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/CoreTracerSyntheticAccessorsTest.java @@ -0,0 +1,13 @@ +package datadog.trace.core; + +import static datadog.trace.test.util.SyntheticAccessors.assertNone; + +import org.junit.jupiter.api.Test; + +/** {@link CoreTracer} is {@code @NoSyntheticAccessors}: {@code CoreSpanBuilder} runs per span. */ +class CoreTracerSyntheticAccessorsTest { + @Test + void nestHasNoSyntheticAccessors() { + assertNone(CoreTracer.class); + } +} diff --git a/internal-api/src/main/java/datadog/perfcontract/NoSyntheticAccessors.java b/internal-api/src/main/java/datadog/perfcontract/NoSyntheticAccessors.java new file mode 100644 index 00000000000..cde2365a881 --- /dev/null +++ b/internal-api/src/main/java/datadog/perfcontract/NoSyntheticAccessors.java @@ -0,0 +1,48 @@ +package datadog.perfcontract; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Declares that no class in the annotated top-level class's nest needs a synthetic accessor. The + * nest is the top-level class and every class nested in it, including inner, local and anonymous + * classes. + * + *

The tracer compiles to Java 8 bytecode, which has no nestmates. When one class of a nest uses + * a {@code private} member of another, javac generates a static {@code access$NNN} method in the + * class that declares the member, and the caller goes through it. Calling another nest member's + * {@code private} constructor also generates a synthetic constructor, with an extra parameter typed + * as a generated class. Each accessor is a real call in the interpreter and C1, and takes an + * inlining level in C2, so on a hot path it can push a callee past {@code MaxInlineLevel}. + * Accessors also add methods and classes to load at startup. + * + *

The fix is to make the member package-private, to stop using it across the nest, or, for a + * member only used off the hot path, to suppress the contract on it. Which side the accessor lands + * on doesn't matter to this contract: it covers the whole nest. + * + *

This marker changes no runtime behavior. + * + *

Checker contract. Inspect the class files of the annotated class's nest: + * + *

    + *
  • Violation: a synthetic method whose name starts with {@code access$}, or a synthetic + * constructor. + *
  • Not a violation: other synthetic members, such as lambda bodies ({@code lambda$}) + * and bridge methods. + *
  • Exemption: an accessor whose target member, the field, method or constructor it + * reaches, carries {@link SuppressPerfContract} naming this contract, directly or through a + * reusable suppression annotation. Use it for members used across the nest only off the hot + * path, so they can stay {@code private}. + *
+ * + *

The check is deterministic: tests call {@code + * datadog.trace.test.util.SyntheticAccessors.assertNone} with the annotated class. + */ +@Documented +@PerfContract +@Retention(RetentionPolicy.CLASS) +@Target(ElementType.TYPE) +public @interface NoSyntheticAccessors {} diff --git a/utils/test-utils/build.gradle.kts b/utils/test-utils/build.gradle.kts index bb2a07b6da7..8349e8eee27 100644 --- a/utils/test-utils/build.gradle.kts +++ b/utils/test-utils/build.gradle.kts @@ -23,6 +23,7 @@ extra["excludedClassesCoverage"] = listOf( "datadog.trace.test.util.FlakySpockExtension*", "datadog.trace.test.util.MultipartRequestParser*", "datadog.trace.test.util.NonRetryable", + "datadog.trace.test.util.SyntheticAccessors", ) dependencies { @@ -46,4 +47,6 @@ dependencies { compileOnly(libs.bundles.spock) testImplementation(libs.junit.platform.launcher) + // @SuppressPerfContract fixtures; the checker matches annotation descriptors and never loads them. + testCompileOnly(project(":internal-api")) } diff --git a/utils/test-utils/src/main/java/datadog/trace/test/util/SyntheticAccessors.java b/utils/test-utils/src/main/java/datadog/trace/test/util/SyntheticAccessors.java new file mode 100644 index 00000000000..3ff4b4aa6bd --- /dev/null +++ b/utils/test-utils/src/main/java/datadog/trace/test/util/SyntheticAccessors.java @@ -0,0 +1,380 @@ +package datadog.trace.test.util; + +import static java.util.Collections.list; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.net.JarURLConnection; +import java.net.URISyntaxException; +import java.net.URL; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.jar.JarEntry; +import java.util.jar.JarFile; +import net.bytebuddy.jar.asm.AnnotationVisitor; +import net.bytebuddy.jar.asm.ClassReader; +import net.bytebuddy.jar.asm.ClassVisitor; +import net.bytebuddy.jar.asm.FieldVisitor; +import net.bytebuddy.jar.asm.MethodVisitor; +import net.bytebuddy.jar.asm.Opcodes; +import net.bytebuddy.jar.asm.Type; + +/** + * Finds the synthetic accessors javac generates when classes in one nest use each other's {@code + * private} members. Backs the checker contract of {@code + * datadog.perfcontract.NoSyntheticAccessors}. + * + *

It reads the nest's class files rather than reflecting on loaded classes, for two reasons: + * only the bytecode says which member an accessor reaches, and {@code @SuppressPerfContract} has + * {@code CLASS} retention, so reflection cannot see it. + */ +public final class SyntheticAccessors { + private static final String SUPPRESS_DESC = "Ldatadog/perfcontract/SuppressPerfContract;"; + private static final String CONTRACT_DESC = "Ldatadog/perfcontract/NoSyntheticAccessors;"; + + private SyntheticAccessors() {} + + /** Fails if any class in {@code topLevel}'s nest has an unsuppressed synthetic accessor. */ + public static void assertNone(Class topLevel) { + List accessors = find(topLevel); + if (!accessors.isEmpty()) { + throw new AssertionError( + "Synthetic accessors in the nest of " + + topLevel.getName() + + "; make the accessed members package-private, or suppress them with" + + " @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = ...):\n " + + String.join("\n ", accessors)); + } + } + + /** + * Lists the unsuppressed synthetic accessors in {@code topLevel}'s nest: methods named {@code + * access$*} and synthetic constructors, each as {@code Class.member -> Owner.target}. + * + *

An accessor is suppressed when the member it reaches carries {@code @SuppressPerfContract} + * naming {@code NoSyntheticAccessors} with a non-blank reason, directly or through an annotation + * that is itself annotated that way. + */ + public static List find(Class topLevel) { + if (topLevel.getEnclosingClass() != null) { + throw new IllegalArgumentException("not a top-level class: " + topLevel.getName()); + } + ClassLoader loader = topLevel.getClassLoader(); + Map nest = new HashMap<>(); + for (String binaryName : nestClassNames(topLevel)) { + NestClass nestClass = NestClass.read(loader, binaryName.replace('.', '/')); + nest.put(nestClass.internalName, nestClass); + } + + List accessors = new ArrayList<>(); + Map suppressingAnnotations = new HashMap<>(); + for (NestClass nestClass : nest.values()) { + for (Accessor accessor : nestClass.accessors) { + Member target = accessor.target == null ? null : resolve(nest, accessor.target); + String problem = suppressionProblem(loader, target, suppressingAnnotations); + if (problem != null) { + accessors.add(accessor.describe(nestClass, target) + problem); + } + } + } + accessors.sort(null); + return accessors; + } + + /** The target as declared in the nest, or {@code null} when it is declared outside it. */ + private static Member resolve(Map nest, MemberRef ref) { + NestClass owner = nest.get(ref.owner); + return owner == null ? null : owner.members.get(ref.key()); + } + + /** + * Returns {@code null} when {@code target} suppresses the contract, else the reason it doesn't, + * formatted as a suffix for the report. + */ + private static String suppressionProblem( + ClassLoader loader, Member target, Map suppressingAnnotations) { + if (target == null) { + return ""; + } + if (target.suppression != null) { + return target.suppression.problem(); + } + for (String annotationDesc : target.annotations) { + if (suppressingAnnotations.computeIfAbsent( + annotationDesc, desc -> isSuppressingAnnotation(loader, desc))) { + return null; + } + } + return ""; + } + + /** Whether the annotation type {@code desc} is meta-annotated with a valid suppression. */ + private static boolean isSuppressingAnnotation(ClassLoader loader, String desc) { + String internalName = Type.getType(desc).getInternalName(); + if (loader.getResource(internalName + ".class") == null) { + return false; + } + Suppression suppression = NestClass.read(loader, internalName).typeSuppression; + return suppression != null && suppression.problem() == null; + } + + /** + * Lists {@code topLevel} and every class file named {@code TopLevel$*} beside it. Anonymous and + * local classes aren't reachable through reflection, so the class files are listed directly. + */ + private static List nestClassNames(Class topLevel) { + String binaryPrefix = topLevel.getName() + "$"; + String filePrefix = topLevel.getSimpleName() + "$"; + URL url = topLevel.getResource(topLevel.getSimpleName() + ".class"); + if (url == null) { + throw new IllegalStateException("no class file for " + topLevel.getName()); + } + List names = new ArrayList<>(); + names.add(topLevel.getName()); + try { + if ("file".equals(url.getProtocol())) { + File[] files = new File(url.toURI()).getParentFile().listFiles(); + if (files != null) { + for (File file : files) { + addNestedName(file.getName(), filePrefix, binaryPrefix, names); + } + } + } else if ("jar".equals(url.getProtocol())) { + String entryDir = topLevel.getName().replace('.', '/'); + entryDir = entryDir.substring(0, entryDir.lastIndexOf('/') + 1); + JarFile jar = ((JarURLConnection) url.openConnection()).getJarFile(); + for (JarEntry entry : list(jar.entries())) { + String name = entry.getName(); + if (name.startsWith(entryDir) && name.indexOf('/', entryDir.length()) < 0) { + addNestedName(name.substring(entryDir.length()), filePrefix, binaryPrefix, names); + } + } + } else { + throw new IllegalStateException("unsupported class location: " + url); + } + } catch (IOException | URISyntaxException e) { + throw new IllegalStateException("cannot list the nest of " + topLevel.getName(), e); + } + return names; + } + + private static void addNestedName( + String fileName, String filePrefix, String binaryPrefix, List names) { + if (fileName.startsWith(filePrefix) && fileName.endsWith(".class")) { + String suffix = fileName.substring(filePrefix.length(), fileName.length() - 6); + names.add(binaryPrefix + suffix); + } + } + + /** A field or method reference: owner, name and descriptor, as in the bytecode. */ + private static final class MemberRef { + final String owner; + final String name; + final String desc; + + MemberRef(String owner, String name, String desc) { + this.owner = owner; + this.name = name; + this.desc = desc; + } + + String key() { + return name + desc; + } + } + + /** A declared field, method or constructor, with the annotations the checker needs. */ + private static final class Member { + final String name; + final List annotations = new ArrayList<>(); + Suppression suppression; + + Member(String name) { + this.name = name; + } + } + + /** An {@code @SuppressPerfContract}'s contracts and reason. */ + private static final class Suppression { + final List contracts = new ArrayList<>(); + String reason; + + /** + * Returns {@code null} when this suppresses {@code NoSyntheticAccessors}, else the reason it + * doesn't, formatted as a report suffix. + */ + String problem() { + if (!contracts.contains(CONTRACT_DESC)) { + return " (its @SuppressPerfContract does not name NoSyntheticAccessors)"; + } + if (reason == null || reason.trim().isEmpty()) { + return " (its @SuppressPerfContract has a blank reason)"; + } + return null; + } + + AnnotationVisitor visitor() { + return new AnnotationVisitor(Opcodes.ASM9) { + @Override + public void visit(String name, Object value) { + if ("reason".equals(name)) { + reason = (String) value; + } + } + + @Override + public AnnotationVisitor visitArray(String name) { + if (!"value".equals(name)) { + return null; + } + return new AnnotationVisitor(Opcodes.ASM9) { + @Override + public void visit(String unused, Object value) { + contracts.add(((Type) value).getDescriptor()); + } + }; + } + }; + } + } + + /** A synthetic accessor and the member it reaches, when that can be told from its bytecode. */ + private static final class Accessor { + final String name; + final String desc; + MemberRef target; + + Accessor(String name, String desc) { + this.name = name; + this.desc = desc; + } + + String describe(NestClass owner, Member resolvedTarget) { + String self = + owner.binaryName() + + ("".equals(name) + ? "." + Type.getArgumentTypes(desc).length + : "." + name); + if (target == null) { + return self; + } + String targetName = resolvedTarget == null ? target.name : resolvedTarget.name; + return self + " -> " + target.owner.replace('/', '.') + "." + targetName; + } + } + + /** The parts of one nest class file the checker needs. */ + private static final class NestClass extends ClassVisitor { + String internalName; + final Map members = new HashMap<>(); + final List accessors = new ArrayList<>(); + Suppression typeSuppression; + + private NestClass() { + super(Opcodes.ASM9); + } + + static NestClass read(ClassLoader loader, String internalName) { + NestClass nestClass = new NestClass(); + try (InputStream in = loader.getResourceAsStream(internalName + ".class")) { + if (in == null) { + throw new IllegalStateException("no class file for " + internalName); + } + new ClassReader(in).accept(nestClass, ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES); + } catch (IOException e) { + throw new IllegalStateException("cannot read " + internalName, e); + } + return nestClass; + } + + String binaryName() { + return internalName.replace('/', '.'); + } + + @Override + public void visit( + int version, + int access, + String name, + String signature, + String superName, + String[] interfaces) { + this.internalName = name; + } + + @Override + public AnnotationVisitor visitAnnotation(String desc, boolean visible) { + if (SUPPRESS_DESC.equals(desc)) { + typeSuppression = new Suppression(); + return typeSuppression.visitor(); + } + return null; + } + + @Override + public FieldVisitor visitField( + int access, String name, String desc, String signature, Object value) { + Member member = new Member(name); + members.put(name + desc, member); + return new FieldVisitor(Opcodes.ASM9) { + @Override + public AnnotationVisitor visitAnnotation(String annotationDesc, boolean visible) { + return annotate(member, annotationDesc); + } + }; + } + + @Override + public MethodVisitor visitMethod( + int access, String name, String desc, String signature, String[] exceptions) { + Member member = new Member(name); + members.put(name + desc, member); + boolean synthetic = (access & Opcodes.ACC_SYNTHETIC) != 0; + Accessor accessor = + synthetic && (name.startsWith("access$") || "".equals(name)) + ? new Accessor(name, desc) + : null; + if (accessor != null) { + accessors.add(accessor); + } + return new MethodVisitor(Opcodes.ASM9) { + @Override + public AnnotationVisitor visitAnnotation(String annotationDesc, boolean visible) { + return annotate(member, annotationDesc); + } + + @Override + public void visitFieldInsn(int opcode, String owner, String fieldName, String fieldDesc) { + if (accessor != null && accessor.target == null) { + accessor.target = new MemberRef(owner, fieldName, fieldDesc); + } + } + + @Override + public void visitMethodInsn( + int opcode, String owner, String methodName, String methodDesc, boolean itf) { + if (accessor == null || accessor.target != null) { + return; + } + // A synthetic constructor reaches the private constructor of its own class. + boolean constructor = "".equals(accessor.name); + if (!constructor || ("".equals(methodName) && owner.equals(internalName))) { + accessor.target = new MemberRef(owner, methodName, methodDesc); + } + } + }; + } + + private static AnnotationVisitor annotate(Member member, String annotationDesc) { + if (SUPPRESS_DESC.equals(annotationDesc)) { + member.suppression = new Suppression(); + return member.suppression.visitor(); + } + member.annotations.add(annotationDesc); + return null; + } + } +} diff --git a/utils/test-utils/src/test/java/datadog/trace/test/util/SyntheticAccessorsTest.java b/utils/test-utils/src/test/java/datadog/trace/test/util/SyntheticAccessorsTest.java new file mode 100644 index 00000000000..b0f79d39f37 --- /dev/null +++ b/utils/test-utils/src/test/java/datadog/trace/test/util/SyntheticAccessorsTest.java @@ -0,0 +1,174 @@ +package datadog.trace.test.util; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.perfcontract.NoSyntheticAccessors; +import datadog.perfcontract.Singleton; +import datadog.perfcontract.SuppressPerfContract; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.util.List; +import java.util.function.Supplier; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class SyntheticAccessorsTest { + @Test + void findsAccessorsForPrivateMembersUsedAcrossTheNest() { + List accessors = SyntheticAccessors.find(WithAccessors.class); + + // WithAccessors.secret is read by Inner, and Inner's private constructor is called by + // WithAccessors: one access$ method and one synthetic constructor. + assertEquals(2, accessors.size(), accessors.toString()); + assertTrue( + accessors.stream().anyMatch(a -> a.contains("WithAccessors.access$")), + accessors.toString()); + assertTrue( + accessors.stream().anyMatch(a -> a.contains("WithAccessors$Inner.")), + accessors.toString()); + assertTrue( + accessors.stream() + .anyMatch(a -> a.endsWith("-> datadog.trace.test.util.WithAccessors.secret")), + accessors.toString()); + assertThrows(AssertionError.class, () -> SyntheticAccessors.assertNone(WithAccessors.class)); + } + + @Test + void acceptsPackagePrivateMembersAndLambdas() { + SyntheticAccessors.assertNone(WithoutAccessors.class); + } + + @Test + void acceptsAccessorsToSuppressedMembers() { + SyntheticAccessors.assertNone(WithSuppressedAccessors.class); + } + + @Test + void acceptsSuppressionsThroughAMetaAnnotation() { + SyntheticAccessors.assertNone(WithMetaSuppressedAccessor.class); + } + + @Test + void rejectsASuppressionWithABlankReason() { + List accessors = SyntheticAccessors.find(WithBlankReasonSuppression.class); + + assertEquals(1, accessors.size(), accessors.toString()); + assertTrue(accessors.get(0).endsWith("(its @SuppressPerfContract has a blank reason)")); + } + + @Test + void rejectsASuppressionOfAnotherContract() { + List accessors = SyntheticAccessors.find(WithOtherContractSuppression.class); + + assertEquals(1, accessors.size(), accessors.toString()); + assertTrue( + accessors + .get(0) + .endsWith("(its @SuppressPerfContract does not name NoSyntheticAccessors)")); + } + + @Test + void readsNestsFromJars() { + // Assertions comes from the junit-jupiter-api jar, so this lists its nest from jar entries. + assertTrue(Assertions.class.getResource("Assertions.class").getProtocol().equals("jar")); + SyntheticAccessors.find(Assertions.class); + } + + @Test + void rejectsNestedClasses() { + assertThrows( + IllegalArgumentException.class, () -> SyntheticAccessors.find(WithAccessors.Inner.class)); + } +} + +final class WithAccessors { + private int secret = 42; + + Inner newInner() { + return new Inner(); + } + + final class Inner { + private Inner() {} + + int read() { + return secret; + } + } +} + +final class WithoutAccessors { + int visible = 42; + + Supplier supplier() { + return () -> visible; + } + + final class Inner { + Inner() {} + + int read() { + return visible; + } + } +} + +final class WithSuppressedAccessors { + @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = "read once, at setup") + private int secret = 42; + + Inner newInner() { + return new Inner(); + } + + final class Inner { + @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = "built once, at setup") + private Inner() {} + + int read() { + return secret; + } + } +} + +/** A reusable suppression, applied through a meta-annotation. */ +@SuppressPerfContract(value = NoSyntheticAccessors.class, reason = "off the hot path") +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.FIELD, ElementType.METHOD, ElementType.CONSTRUCTOR}) +@interface ColdPath {} + +final class WithMetaSuppressedAccessor { + @ColdPath private int secret = 42; + + final class Inner { + int read() { + return secret; + } + } +} + +final class WithBlankReasonSuppression { + @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = " ") + private int secret = 42; + + final class Inner { + int read() { + return secret; + } + } +} + +final class WithOtherContractSuppression { + @SuppressPerfContract(value = Singleton.class, reason = "not this contract") + private int secret = 42; + + final class Inner { + int read() { + return secret; + } + } +}