From 33780e0ca4cfebf99f34b0dd6f5169079a2b6e1a Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Tue, 12 Mar 2024 08:29:24 +0800 Subject: [PATCH] Consolidates notes on "extra" data Before this change, notes about state, particularly the "one instance per plugin" rule were not consolidated on the method that can cause problems `addExtra`. This migrates notes together with additional warnings that when we talk about this being used for propagation plugins, we also mean not for ad-hoc usage, as that can lead to excessive overhead. See #1421 Signed-off-by: Adrian Cole --- .../java/brave/baggage/BaggageFields.java | 4 +- .../java/brave/propagation/TraceContext.java | 42 ++++++++++++++----- .../TraceContextOrSamplingFlags.java | 13 +++++- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/brave/src/main/java/brave/baggage/BaggageFields.java b/brave/src/main/java/brave/baggage/BaggageFields.java index fa3e991b02..0b51322a6b 100644 --- a/brave/src/main/java/brave/baggage/BaggageFields.java +++ b/brave/src/main/java/brave/baggage/BaggageFields.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2024 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -25,7 +25,7 @@ *

Built-in fields

* The following are fields that dispatch to methods on the {@link TraceContext}. They are available * regardless of {@link BaggagePropagation}. None will return in lookups such as {@link - * BaggageField#getAll(TraceContext)} or {@link BaggageField#getByName(TraceContext, String)} + * BaggageField#getAllValues(TraceContext)} or {@link BaggageField#getByName(TraceContext, String)}. * *

    *
  1. {@link #TRACE_ID}
    2. diff --git a/brave/src/main/java/brave/propagation/TraceContext.java b/brave/src/main/java/brave/propagation/TraceContext.java index 3c1c537ed9..65bb0f3f6c 100644 --- a/brave/src/main/java/brave/propagation/TraceContext.java +++ b/brave/src/main/java/brave/propagation/TraceContext.java @@ -14,9 +14,12 @@ package brave.propagation; import brave.Span; +import brave.baggage.BaggageFields; +import brave.baggage.BaggagePropagation; import brave.internal.InternalPropagation; import brave.internal.Nullable; import brave.internal.Platform; +import brave.internal.baggage.ExtraBaggageContext; import java.lang.ref.WeakReference; import java.util.Collections; import java.util.List; @@ -182,15 +185,15 @@ public boolean shared() { } /** - * Returns a list of additional data propagated through this trace. + * Used internally by propagation plugins to search for their instance of state regardless of if + * it is in a {@linkplain TraceContext} or {@linkplain TraceContextOrSamplingFlags}. * - *

    The contents are intentionally opaque, deferring to {@linkplain Propagation} to define. An - * example implementation could be storing a class containing a correlation value, which is - * extracted from incoming requests and injected as-is onto outgoing requests. - * - *

    Implementations are responsible for scoping any data stored here. This can be performed - * when {@link Propagation.Factory#decorate(TraceContext)} is called. + *

    Tools that work only on {@linkplain TraceContext} should use {@link #findExtra(Class)} + * instead. * + * @see #findExtra(Class) + * @see TraceContextOrSamplingFlags#extra() + * @see Builder#addExtra(Object) for notes on extra values. * @since 4.9 */ public List extra() { @@ -198,11 +201,10 @@ public List extra() { } /** - * Returns a {@linkplain #extra() propagated state} of the given type if present or null if not. + * Used internally by propagation plugins to search for their instance of state. * - *

    Note: It is the responsibility of {@link Propagation.Factory#decorate(TraceContext)} - * to consolidate elements. If it doesn't, there could be multiple instances of a given type and - * this can break logic. + * @see #extra() + * @see Builder#addExtra(Object) for notes on extra values. */ @Nullable public T findExtra(Class type) { return findExtra(type, extraList); @@ -372,6 +374,24 @@ public Builder clearExtra() { } /** + * This is an advanced function used for {@link Propagation} plugins, such as + * {@link BaggagePropagation}, to add an internal object to hold state when it isn't already in + * {@linkplain #extra()}. + * + *

    The "extra" parameter is intentionally opaque, and should be an internal type defined by + * a {@linkplain Propagation} plugin. An example implementation could be storing a class + * containing a correlation value, which is extracted from incoming requests and injected as-is + * onto outgoing requests. A real world use is {@link BaggageFields}, which holds all baggage + * fields to propagate in one instance. + * + *

    Note: It is the responsibility of {@link Propagation.Factory#decorate(TraceContext)} + * to consolidate elements (by searching for an existing instance of their state with {@link + * #findExtra(Class)} or {@link #extra()}). If it doesn't, there could be multiple instances of + * a given type and this can break logic and add overhead. Decoration is also when + * implementations define their scope. For example, a plugin that only propagates a field + * without changing will be constant for the whole request. A plugin that allows changes of + * state need careful coding, such as done in {@link BaggagePropagation}. + * * @see #extra() * @since 5.12 */ diff --git a/brave/src/main/java/brave/propagation/TraceContextOrSamplingFlags.java b/brave/src/main/java/brave/propagation/TraceContextOrSamplingFlags.java index 8e21405e83..35394525a7 100644 --- a/brave/src/main/java/brave/propagation/TraceContextOrSamplingFlags.java +++ b/brave/src/main/java/brave/propagation/TraceContextOrSamplingFlags.java @@ -14,6 +14,7 @@ package brave.propagation; import brave.Tracer; +import brave.baggage.BaggagePropagation; import brave.internal.InternalPropagation; import brave.internal.Nullable; import brave.propagation.TraceContext.Extractor; @@ -245,9 +246,10 @@ public TraceContextOrSamplingFlags sampled(boolean sampled) { * #context()} is {@code null}. * * @see TraceContext#extra() + * @see Builder#addExtra(Object) for notes on extra values. * @since 4.9 */ - public final List extra() { + public List extra() { return extraList; } @@ -309,7 +311,16 @@ public Builder sampledLocal() { } /** + * This is an advanced function used for {@link Propagation} plugins, such as + * {@link BaggagePropagation}, to add an internal object to hold state before extracting a + * remote request. + * + *

    Implications of data are the same as {@link TraceContext.Builder#addExtra(Object)}. The + * main difference here is that {@link Extractor#extract(Object)} may not result in a trace + * context. For example, baggage fields can exist without an incoming trace. + * * @see TraceContextOrSamplingFlags#extra() + * @see TraceContext.Builder#addExtra(Object) * @since 4.9 */ public Builder addExtra(Object extra) {