Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 @@ -105,6 +105,11 @@ public void setEnabled(boolean enabled) {
}
}

@Override
public boolean isEnabled() {
return enabled;
}

public void setIncludeNames(List<String> includeNames) {
this.includeNames = includeNames;
this.filterAutomaton = buildAutomaton(includeNames, excludeNames);
Expand Down
54 changes: 54 additions & 0 deletions server/src/main/java/org/elasticsearch/telemetry/Trace.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.telemetry;

import org.elasticsearch.common.UUIDs;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.telemetry.tracing.Traceable;
import org.elasticsearch.telemetry.tracing.Tracer;
import org.elasticsearch.threadpool.ThreadPool;
Copy link
Contributor

Choose a reason for hiding this comment

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

so I am planning to move the telemetry package into its own :lib this import will make it impossible.


import java.util.Map;

public class Trace {

public static Releasable trace(ThreadPool threadPool, Tracer tracer, String name) {
return trace(threadPool, tracer, name, Map.of());
}

/**
* Creates a new trace for a synchronous block of code.
* @return a span that needs to be released once block of code is completed
*/
public static Releasable trace(ThreadPool threadPool, Tracer tracer, String name, Map<String, Object> attributes) {
if (tracer.isEnabled() == false) {
return () -> {};
}
var span = Span.create();
var ctx = threadPool.getThreadContext().newTraceContext();
tracer.startTrace(threadPool.getThreadContext(), span, name, attributes);
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

First off: this is great and allows to create sub-spans of the current thread context very easily.

However, I wonder if we should use threadPool.getThreadContext().newTraceContext(); this mutates the current thread local state, and in particular only works if there's a strict hierarchy of spans. @elastic/es-core-infra what do you think?

It might be sufficient to copy the logic from newTraceContext to create a new, separate TraceContext and store it as part of the Releaseable that is returned here. I attempted that here and it seemed to work fine.

This would allow create interleaved traces in the same thread which would provide more flexibility; my use case for this is seeing when individual ESQL operators start and stop, even though they are executed concurrently on the same thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am worried about the use of threadpool & threadcontext here.
we should aim to use org.eleasticsearch.telemetry.tracing.TraceContext
I think it should be possible to either:

  • refactor the Pr to have the new context and context to restore (would be probably a bit ugly)
  • refactor the TraceContext to create a newTraceContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor the Pr to have the new context and context to restore (would be probably a bit ugly)

Not sure I follow, threadPool.getThreadContext().newTraceContext() creates a new context and returns a releasable to rollback to the previous one.
To me it sounds like what is suggested, however implicit (may be the comment is about it?)

Copy link
Contributor

Choose a reason for hiding this comment

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

refactor the Pr to have the new context and context to restore (would be probably a bit ugly)

as part of a method signature.
My point is that we should not try to add ThreadContext in the telemetry api


return () -> {
tracer.stopTrace(span);
ctx.restore();
};
}

private record Span(String spanId) implements Traceable {

@Override
public String getSpanId() {
return spanId;
}

public static Span create() {
return new Span(UUIDs.randomBase64UUID());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
*/
public interface Tracer {

/**
* @return {@code if service is enabled}
*/
boolean isEnabled();

/**
* Called when a span starts.
* @param traceContext the current context. Required for tracing parent/child span activity.
Expand Down Expand Up @@ -145,6 +150,12 @@ public interface Tracer {
* in order to avoid null checks everywhere.
*/
Tracer NOOP = new Tracer() {

@Override
public boolean isEnabled() {
return false;
}

@Override
public void startTrace(TraceContext traceContext, Traceable traceable, String name, Map<String, Object> attributes) {}

Expand Down