Skip to content
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.

Sign up for GitHub

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

Add Traceable interface #80788

Merged
merged 4 commits into from
Nov 17, 2021
Merged

Add Traceable interface #80788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0139b87
Add Traceable interface
ywangd Nov 17, 2021
10b3ed4
Merge remote-tracking branch 'origin/feature/apm-integration' into ap…
ywangd Nov 17, 2021
2271b1f
address feedback
ywangd Nov 17, 2021
bcd4d6d
address more feedback
ywangd Nov 17, 2021
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
14 changes: 11 additions & 3 deletions server/src/main/java/org/elasticsearch/plugins/TracingPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,21 @@

package org.elasticsearch.plugins;

import org.elasticsearch.tasks.Task;
import java.util.Map;

public interface TracingPlugin {

interface Traceable {
String getSpanId();

String getSpanName();

Map<String, Object> getAttributes();
}

interface Tracer {
void onTaskRegistered(Task task);
void onRegistered(Traceable traceable);

void onTaskUnregistered(Task task);
void onUnregistered(Traceable traceable);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe registered/unregistered is still related to the task naming.
Should generic methods be called onSpanStarted/onStarted or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I changed the method names to onTraceStarted and onTraceStopped.

}
}
18 changes: 17 additions & 1 deletion server/src/main/java/org/elasticsearch/tasks/Task.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.io.stream.NamedWriteable;
import org.elasticsearch.plugins.TracingPlugin;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.ToXContentObject;

Expand All @@ -20,7 +21,7 @@
/**
* Current task information
*/
public class Task {
public class Task implements TracingPlugin.Traceable {

/**
* The request header to mark tasks with specific ids
Expand Down Expand Up @@ -220,4 +221,19 @@ public TaskResult result(DiscoveryNode node, ActionResponse response) throws IOE
throw new IllegalStateException("response has to implement ToXContent to be able to store the results");
}
}

@Override
public String getSpanId() {
return String.valueOf(id);
}

@Override
public String getSpanName() {
return action;
}

@Override
public Map<String, Object> getAttributes() {
return Map.of("es.task.id", id);
}
}
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/tasks/TaskTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public void addTracer(TracingPlugin.Tracer tracer) {
public void onTaskRegistered(Task task) {
for (TracingPlugin.Tracer tracer : tracers) {
try {
tracer.onTaskRegistered(task);
tracer.onRegistered(task);
} catch (Exception e) {
assert false : e;
logger.warn(
Expand All @@ -50,7 +50,7 @@ public void onTaskRegistered(Task task) {
public void onTaskUnregistered(Task task) {
for (TracingPlugin.Tracer tracer : tracers) {
try {
tracer.onTaskUnregistered(task);
tracer.onUnregistered(task);
} catch (Exception e) {
assert false : e;
logger.warn(
Expand Down
32 changes: 24 additions & 8 deletions x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.common.component.AbstractLifecycleComponent;
import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
import org.elasticsearch.plugins.TracingPlugin;
import org.elasticsearch.tasks.Task;

import java.util.ArrayList;
import java.util.Collection;
Expand All @@ -34,7 +33,7 @@ public class APMTracer extends AbstractLifecycleComponent implements TracingPlug

public static final CapturingSpanExporter CAPTURING_SPAN_EXPORTER = new CapturingSpanExporter();

private final Map<Long, Span> taskSpans = ConcurrentCollections.newConcurrentMap();
private final Map<String, Span> spans = ConcurrentCollections.newConcurrentMap();

private volatile Tracer tracer;

Expand All @@ -60,20 +59,37 @@ protected void doStop() {}
protected void doClose() {}

@Override
public void onTaskRegistered(Task task) {
public void onRegistered(TracingPlugin.Traceable traceable) {
final Tracer tracer = this.tracer;
if (tracer != null) {
taskSpans.computeIfAbsent(task.getId(), taskId -> {
final Span span = tracer.spanBuilder(task.getAction()).startSpan();
span.setAttribute("es.task.id", task.getId());
spans.computeIfAbsent(traceable.getSpanId(), spanId -> {
final Span span = tracer.spanBuilder(traceable.getSpanName()).startSpan();
Copy link

Choose a reason for hiding this comment

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

When setting attributes while creating the span, it's common to use SpanBuilder.setAttribute(key, value). It would look like:

SpanBuilder spanBuilder = tracer.spanBuilder(traceable.getSpanName());
for (Map.Entry<String, Object> entry : traceable.getAttributes().entrySet()) {
    Object value = entry.getValue();
    if (value instanceof String) {
        spanBuilder.setAttribute(entry.getKey(), (String) value);
    } else if (value instanceof Long) {
        spanBuilder.setAttribute(entry.getKey(), (Long) value);
    } else if (value instanceof Integer) {
        spanBuilder.setAttribute(entry.getKey(), (Integer) value);
    } else if (value instanceof Double) {
        spanBuilder.setAttribute(entry.getKey(), (Double) value);
    } else if (value instanceof Boolean) {
        spanBuilder.setAttribute(entry.getKey(), (Boolean) value);
    } else {
        throw new IllegalArgumentException(
            "span attributes do not support value type of [" + value.getClass().getCanonicalName() + "]"
    );
}
return spanBuilder.startSpan();

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I updated as suggested.

for (Map.Entry<String, Object> entry : traceable.getAttributes().entrySet()) {
final Object value = entry.getValue();
if (value instanceof String) {
span.setAttribute(entry.getKey(), (String) value);
} else if (value instanceof Long) {
span.setAttribute(entry.getKey(), (Long) value);
} else if (value instanceof Integer) {
span.setAttribute(entry.getKey(), (Integer) value);
} else if (value instanceof Double) {
span.setAttribute(entry.getKey(), (Double) value);
} else if (value instanceof Boolean) {
span.setAttribute(entry.getKey(), (Boolean) value);
} else {
throw new IllegalArgumentException(
"span attributes do not support value type of [" + value.getClass().getCanonicalName() + "]"
);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not pretty. But it does the job for now and keep other parts unware of the opentelemetry dependencies. We can improve it once we know more about what other things needed by the new Traceable interface.

return span;
});
}
}

@Override
public void onTaskUnregistered(Task task) {
final Span span = taskSpans.remove(task.getId());
public void onUnregistered(TracingPlugin.Traceable traceable) {
final Span span = spans.remove(traceable.getSpanId());
if (span != null) {
span.end();
}
Expand Down