Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ private Builder getSpanBuilder(String spanName, Context context) {
private <T> T getOrDefault(Context context, String key, T defaultValue, Class<T> clazz) {
final Optional<Object> optional = context.getData(key);
final Object result = optional.filter(value -> clazz.isAssignableFrom(value.getClass())).orElseGet(() -> {
logger.warning("Could not extract key '{}' of type '{}' from context.", key, clazz);
logger.verbose("Could not extract key '{}' of type '{}' from context.", key, clazz);
return defaultValue;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
import com.azure.core.util.Context;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.ServiceLoader;

/**
Expand All @@ -17,14 +17,13 @@
*/
public final class TracerProxy {

private static final List<Tracer> TRACERS;
private static Tracer TRACER;

static {
ServiceLoader<Tracer> serviceLoader = ServiceLoader.load(Tracer.class);
List<Tracer> tracers = new ArrayList<>();
for (Tracer tracer : serviceLoader) {
tracers.add(tracer);
if (serviceLoader != null) {
TRACER = serviceLoader.iterator().next();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any mechanisms available which will ensure a certain tracer is loaded if I happen to bring multiple? This can be a follow-up as it is definitely an edge case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we could add a feature where a user preference could be preferred for a single implementation if multiple found.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if there are multiple instances of tracer implementations, should we support all of them? Create spans on each tracer implementation?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no real use case to support multiple implementations, as users would always tie it with a single implementation of tracer to an exporter.
It brings in a lot of complexity for the users of the library to actually look to configure each span with a single tracer implementation and then the corresponding exporter for that tracer. As mentioned above, not a real use case and something only Java SDK was supporting.

}
TRACERS = Collections.unmodifiableList(tracers);
}

private TracerProxy() {
Expand All @@ -33,7 +32,7 @@ private TracerProxy() {

/**
* A new tracing span is created for each {@link Tracer tracer} plugged into the SDK.
*
* <p>
* The {@code context} will be checked for information about a parent span. If a parent span is found, the new span
* will be added as a child. Otherwise, the parent span will be created and added to the {@code context} and any
* downstream {@code start()} calls will use the created span as the parent.
Expand All @@ -44,12 +43,10 @@ private TracerProxy() {
* @return An updated {@link Context} object.
*/
public static Context start(String methodName, Context context) {
Context local = context;
for (Tracer tracer : TRACERS) {
local = tracer.start(methodName, local);
if (TRACER == null) {
return context;
}

return local;
return TRACER.start(methodName, context);
}

/**
Expand All @@ -61,7 +58,10 @@ public static Context start(String methodName, Context context) {
* @param context Additional metadata that is passed through the call stack.
*/
public static void setAttribute(String key, String value, Context context) {
TRACERS.forEach(tracer -> tracer.setAttribute(key, value, context));
if (TRACER == null) {
return;
}
TRACER.setAttribute(key, value, context);
}

/**
Expand All @@ -72,7 +72,10 @@ public static void setAttribute(String key, String value, Context context) {
* @param context Additional metadata that is passed through the call stack.
*/
public static void end(int responseCode, Throwable error, Context context) {
TRACERS.forEach(tracer -> tracer.end(responseCode, error, context));
if (TRACER == null) {
return;
}
TRACER.end(responseCode, error, context);
}

/**
Expand All @@ -84,11 +87,9 @@ public static void end(int responseCode, Throwable error, Context context) {
* @return An updated {@link Context} object.
*/
public static Context setSpanName(String spanName, Context context) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this API return Context? It's inconsistent with setAttribute() API which returns a void. Or, should setAttribute() return Context too since start() is also returning Context?

@samvaity samvaity May 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start() should return a Context as we update the context to hold the span that it just created/started.
setAttribute() shouldn't because we do not have anything to return. We are actually just using the span information from the provided Context.
setSpanName() I think this method is redundant to set the span name as we already use the provided context for that. I see it is only used in RestProxy here.

Context local = context;
for (Tracer tracer : TRACERS) {
local = tracer.setSpanName(spanName, context);
if (TRACER == null) {
return context;
}

return local;
return TRACER.setSpanName(spanName, context);
}
}