-
Notifications
You must be signed in to change notification settings - Fork 48
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
Added an option to have baggage through Observation API #688
Changes from 2 commits
7c5d17f
0592488
544aa1c
085e916
464d419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/** | ||
* Copyright 2022 the original author or authors. | ||
* Copyright 2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -22,6 +22,7 @@ | |
import io.micrometer.tracing.*; | ||
|
||
import java.io.Closeable; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
@@ -36,15 +37,25 @@ public class BraveBaggageManager implements Closeable, BaggageManager { | |
|
||
private final List<String> tagFields; | ||
|
||
private final List<String> remoteFields; | ||
|
||
@Nullable | ||
private Tracer tracer; | ||
|
||
/** | ||
* Create an instance of {@link BraveBaggageManager}. | ||
* @param tagFields fields of baggage keys that should become tags on a span | ||
* @param remoteFields fields of baggage keys that should be propagated over the wire | ||
*/ | ||
public BraveBaggageManager(List<String> tagFields) { | ||
public BraveBaggageManager(List<String> tagFields, List<String> remoteFields) { | ||
this.tagFields = tagFields; | ||
this.remoteFields = remoteFields(tagFields, remoteFields); | ||
} | ||
|
||
private static List<String> remoteFields(List<String> tagFields, List<String> remoteFields) { | ||
List<String> combined = new ArrayList<>(tagFields); | ||
combined.addAll(remoteFields); | ||
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. Why are these combined? It looks like the other baggage managers don't combine them. 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. So when you have tag fields it means that it's baggage that additionally will be tagged. Inc case of Brave we never had a constructor that accepts just remote fields and I can't introduce one cause we have a constructor for tag fields already. So it does make sense to combine them. Of course I should combine those for other baggage managers too - I'll look into that, thanks 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. Is it better to create a unique set of fields? 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. I think I'm still confused. The assumption seems to be that all tagFields are remote baggage. Is local baggage that should also be tagged not possible? In the test code, it looks like the baggage key to be tagged is local baggage:
So shouldn't that not be combined with remote fields? 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. From the point of view of a baggage manager there is no concept of local fields. That is really brave specific and has to be configured via propagation configuration. I understand that this might be confusing. We can rename this from 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. That sounds good. |
||
return combined; | ||
} | ||
|
||
/** | ||
|
@@ -53,6 +64,16 @@ public BraveBaggageManager(List<String> tagFields) { | |
*/ | ||
public BraveBaggageManager() { | ||
this.tagFields = Collections.emptyList(); | ||
this.remoteFields = Collections.emptyList(); | ||
} | ||
|
||
/** | ||
* Create an instance of {@link BraveBaggageManager}. | ||
* @param tagFields fields of baggage keys that should become tags on a span | ||
*/ | ||
public BraveBaggageManager(List<String> tagFields) { | ||
this.tagFields = tagFields; | ||
this.remoteFields = new ArrayList<>(tagFields); | ||
} | ||
|
||
@Override | ||
|
@@ -136,4 +157,9 @@ void setTracer(Tracer tracer) { | |
this.tracer = tracer; | ||
} | ||
|
||
@Override | ||
public List<String> getRemoteFields() { | ||
return this.remoteFields; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/** | ||
* Copyright 2022 the original author or authors. | ||
* Copyright 2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -27,13 +27,17 @@ | |
import io.micrometer.common.util.internal.logging.InternalLogger; | ||
import io.micrometer.common.util.internal.logging.InternalLoggerFactory; | ||
import io.micrometer.context.ContextRegistry; | ||
import io.micrometer.observation.Observation; | ||
import io.micrometer.observation.Observation.Scope; | ||
import io.micrometer.observation.ObservationRegistry; | ||
import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; | ||
import io.micrometer.tracing.*; | ||
import io.micrometer.tracing.contextpropagation.ObservationAwareSpanThreadLocalAccessor; | ||
import io.micrometer.tracing.handler.DefaultTracingObservationHandler; | ||
import org.assertj.core.api.Assertions; | ||
import org.junit.jupiter.api.AfterAll; | ||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import reactor.core.observability.micrometer.Micrometer; | ||
import reactor.core.publisher.Hooks; | ||
|
@@ -42,6 +46,7 @@ | |
import reactor.core.scheduler.Schedulers; | ||
|
||
import java.time.Duration; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
@@ -61,6 +66,10 @@ class BaggageTests { | |
|
||
public static final String TAG_VALUE = "tagValue"; | ||
|
||
public static final String OBSERVATION_BAGGAGE_KEY = "observationKey"; | ||
|
||
public static final String OBSERVATION_BAGGAGE_VALUE = "observationValue"; | ||
|
||
TestSpanHandler spanHandler = new TestSpanHandler(); | ||
|
||
StrictCurrentTraceContext braveCurrentTraceContext = StrictCurrentTraceContext.create(); | ||
|
@@ -73,6 +82,7 @@ class BaggageTests { | |
.traceId128Bit(true) | ||
.propagationFactory(BaggagePropagation.newFactoryBuilder(B3Propagation.FACTORY) | ||
.add(BaggagePropagationConfig.SingleBaggageField.remote(BaggageField.create(KEY_1))) | ||
.add(BaggagePropagationConfig.SingleBaggageField.remote(BaggageField.create(OBSERVATION_BAGGAGE_KEY))) | ||
.add(BaggagePropagationConfig.SingleBaggageField.local(BaggageField.create(TAG_KEY))) | ||
.build()) | ||
.sampler(Sampler.ALWAYS_SAMPLE) | ||
|
@@ -84,10 +94,15 @@ class BaggageTests { | |
BravePropagator propagator = new BravePropagator(tracing); | ||
|
||
Tracer tracer = new BraveTracer(this.braveTracer, this.bridgeContext, | ||
new BraveBaggageManager(Collections.singletonList(TAG_KEY))); | ||
new BraveBaggageManager(Collections.singletonList(TAG_KEY), Arrays.asList(KEY_1, OBSERVATION_BAGGAGE_KEY))); | ||
|
||
ObservationRegistry observationRegistry = ObservationThreadLocalAccessor.getInstance().getObservationRegistry(); | ||
|
||
@BeforeEach | ||
void setupHandler() { | ||
observationRegistry.observationConfig().observationHandler(new DefaultTracingObservationHandler(tracer)); | ||
} | ||
|
||
@AfterEach | ||
void cleanup() { | ||
tracing.close(); | ||
|
@@ -258,6 +273,30 @@ void baggageTagKey() { | |
then(mutableSpan.tags().get(TAG_KEY)).isEqualTo(TAG_VALUE); | ||
} | ||
|
||
@Test | ||
void baggageWithObservationApiWithRemoteFields() { | ||
Observation observation = Observation.start("foo", observationRegistry) | ||
.lowCardinalityKeyValue(KEY_1, TAG_VALUE) | ||
.highCardinalityKeyValue(OBSERVATION_BAGGAGE_KEY, OBSERVATION_BAGGAGE_VALUE); | ||
then(tracer.getBaggage(KEY_1).get()).isNull(); | ||
then(tracer.getBaggage(OBSERVATION_BAGGAGE_KEY).get()).isNull(); | ||
|
||
try (Scope scope = observation.openScope()) { | ||
then(tracer.getBaggage(KEY_1).get()).isEqualTo(TAG_VALUE); | ||
then(tracer.getBaggage(OBSERVATION_BAGGAGE_KEY).get()).isEqualTo(OBSERVATION_BAGGAGE_VALUE); | ||
} | ||
|
||
then(tracer.currentSpan()).isNull(); | ||
then(tracer.getBaggage(KEY_1).get()).isNull(); | ||
then(tracer.getBaggage(OBSERVATION_BAGGAGE_KEY).get()).isNull(); | ||
Comment on lines
+290
to
+291
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. Can we do these check before opening the scope too? 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. Of course |
||
observation.stop(); | ||
|
||
then(spanHandler.spans()).hasSize(1); | ||
MutableSpan mutableSpan = spanHandler.spans().get(0); | ||
then(mutableSpan.tags().get(KEY_1)).isEqualTo(TAG_VALUE); | ||
then(mutableSpan.tags().get(OBSERVATION_BAGGAGE_KEY)).isEqualTo(OBSERVATION_BAGGAGE_VALUE); | ||
} | ||
|
||
@Test | ||
void baggageTagKeyWithLegacyApi() { | ||
ScopedSpan span = this.tracer.startScopedSpan("call1"); | ||
|
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.
Shouldn't we introduce a new ctor?
Can any of these be
null
?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.
We can't - type inference is the same. They can't be null, we have non null api by default. We can pass empty collections