Skip to content

Commit 00d4b50

Browse files
pugnascotiapugnascotia
committed
Actually shift work to call sites
1 parent f0f0d77 commit 00d4b50

10 files changed

Lines changed: 143 additions & 160 deletions

File tree

‎server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ public void sendResponse(RestResponse restResponse) {
8787
// the request content again and release it
8888
httpRequest.release();
8989

90+
final String traceId = "rest-" + this.request.getRequestId();
91+
9092
final ArrayList<Releasable> toClose = new ArrayList<>(3);
9193
if (HttpUtils.shouldCloseConnection(httpRequest)) {
9294
toClose.add(() -> CloseableChannel.closeChannel(httpChannel));
@@ -97,7 +99,7 @@ public void sendResponse(RestResponse restResponse) {
9799
String contentLength = null;
98100
final Runnable onFinish = () -> {
99101
Releasables.close(toClose);
100-
tracer.onTraceStopped(this);
102+
tracer.onTraceStopped(traceId);
101103
};
102104

103105
try {
@@ -138,9 +140,9 @@ public void sendResponse(RestResponse restResponse) {
138140

139141
addCookies(httpResponse);
140142

141-
tracer.setAttribute(this, "http.status_code", restResponse.status().getStatus());
143+
tracer.setAttribute(traceId, "http.status_code", restResponse.status().getStatus());
142144
restResponse.getHeaders()
143-
.forEach((key, values) -> tracer.setAttribute(this, "http.response.headers." + key, String.join("; ", values)));
145+
.forEach((key, values) -> tracer.setAttribute(traceId, "http.response.headers." + key, String.join("; ", values)));
144146

145147
ActionListener<Void> listener = ActionListener.wrap(onFinish);
146148
try (ThreadContext.StoredContext existing = threadContext.stashContext()) {

‎server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java‎

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@
2323
import java.io.OutputStream;
2424
import java.io.UncheckedIOException;
2525
import java.util.Collections;
26-
import java.util.HashMap;
27-
import java.util.Locale;
2826
import java.util.Map;
2927
import java.util.Set;
3028
import java.util.function.Predicate;
@@ -48,8 +46,6 @@ public abstract class AbstractRestChannel implements RestChannel {
4846

4947
private BytesStream bytesOut;
5048

51-
private String tracePath;
52-
5349
/**
5450
* Construct a channel for handling the request.
5551
*
@@ -203,46 +199,4 @@ public RestRequest request() {
203199
public boolean detailedErrorsEnabled() {
204200
return detailedErrorsEnabled;
205201
}
206-
207-
@Override
208-
public String getTracePath() {
209-
return tracePath;
210-
}
211-
212-
@Override
213-
public void setTracePath(String tracePath) {
214-
this.tracePath = tracePath;
215-
}
216-
217-
@Override
218-
public String getSpanId() {
219-
return "rest-" + this.request().getRequestId();
220-
}
221-
222-
@Override
223-
public String getSpanName() {
224-
final String tracePath = this.getTracePath();
225-
return this.request().method() + " " + (tracePath != null ? tracePath : this.request().path());
226-
}
227-
228-
@Override
229-
public Map<String, Object> getAttributes() {
230-
final RestRequest req = this.request();
231-
Map<String, Object> attributes = new HashMap<>();
232-
req.getHeaders().forEach((key, values) -> {
233-
final String lowerKey = key.toLowerCase(Locale.ROOT).replace('-', '_');
234-
final String value = switch (lowerKey) {
235-
case "authorization", "cookie", "secret", "session", "set_cookie", "token" -> "[REDACTED]";
236-
default -> String.join("; ", values);
237-
};
238-
attributes.put("http.request.headers." + lowerKey, value);
239-
});
240-
attributes.put("http.method", req.method().name());
241-
attributes.put("http.url", req.uri());
242-
switch (req.getHttpRequest().protocolVersion()) {
243-
case HTTP_1_0 -> attributes.put("http.flavour", "1.0");
244-
case HTTP_1_1 -> attributes.put("http.flavour", "1.1");
245-
}
246-
return attributes;
247-
}
248202
}

‎server/src/main/java/org/elasticsearch/rest/RestChannel.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
import org.elasticsearch.common.io.stream.BytesStream;
1212
import org.elasticsearch.core.Nullable;
13-
import org.elasticsearch.tracing.Traceable;
1413
import org.elasticsearch.xcontent.XContentBuilder;
1514
import org.elasticsearch.xcontent.XContentType;
1615

@@ -19,7 +18,7 @@
1918
/**
2019
* A channel used to construct bytes / builder based outputs, and send responses.
2120
*/
22-
public interface RestChannel extends Traceable {
21+
public interface RestChannel {
2322

2423
XContentBuilder newBuilder() throws IOException;
2524

@@ -40,8 +39,4 @@ XContentBuilder newBuilder(@Nullable XContentType xContentType, @Nullable XConte
4039
boolean detailedErrorsEnabled();
4140

4241
void sendResponse(RestResponse response);
43-
44-
void setTracePath(String path);
45-
46-
String getTracePath();
4742
}

‎server/src/main/java/org/elasticsearch/rest/RestController.java‎

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@
3636
import java.io.ByteArrayOutputStream;
3737
import java.io.IOException;
3838
import java.io.InputStream;
39+
import java.util.HashMap;
3940
import java.util.HashSet;
4041
import java.util.Iterator;
4142
import java.util.List;
43+
import java.util.Locale;
4244
import java.util.Map;
4345
import java.util.Set;
4446
import java.util.concurrent.atomic.AtomicBoolean;
@@ -418,22 +420,67 @@ private boolean handleNoHandlerFound(
418420
final Set<RestRequest.Method> validMethodSet = getValidHandlerMethodSet(rawPath);
419421
if (validMethodSet.contains(method) == false) {
420422
if (method == RestRequest.Method.OPTIONS) {
421-
tracer.onTraceStarted(threadContext, channel);
423+
startTrace(threadContext, channel);
422424
handleOptionsRequest(channel, validMethodSet);
423425
return true;
424426
}
425427
if (validMethodSet.isEmpty() == false) {
426428
// If an alternative handler for an explicit path is registered to a
427429
// different HTTP method than the one supplied - return a 405 Method
428430
// Not Allowed error.
429-
tracer.onTraceStarted(threadContext, channel);
431+
startTrace(threadContext, channel);
430432
handleUnsupportedHttpMethod(uri, method, channel, validMethodSet, null);
431433
return true;
432434
}
433435
}
434436
return false;
435437
}
436438

439+
private void startTrace(ThreadContext threadContext, RestChannel channel) {
440+
startTrace(threadContext, channel, null);
441+
}
442+
443+
private void startTrace(ThreadContext threadContext, RestChannel channel, String restPath) {
444+
final RestRequest req = channel.request();
445+
if (restPath == null) {
446+
restPath = req.path();
447+
}
448+
String method = null;
449+
try {
450+
method = req.method().name();
451+
} catch (IllegalArgumentException e) {
452+
// Invalid methods throw an exception
453+
}
454+
String name;
455+
if (method != null) {
456+
name = method + " " + restPath;
457+
} else {
458+
name = restPath;
459+
}
460+
461+
Map<String, Object> attributes = new HashMap<>();
462+
req.getHeaders().forEach((key, values) -> {
463+
final String lowerKey = key.toLowerCase(Locale.ROOT).replace('-', '_');
464+
final String value = switch (lowerKey) {
465+
case "authorization", "cookie", "secret", "session", "set_cookie", "token" -> "[REDACTED]";
466+
default -> String.join("; ", values);
467+
};
468+
attributes.put("http.request.headers." + lowerKey, value);
469+
});
470+
attributes.put("http.method", method);
471+
attributes.put("http.url", req.uri());
472+
switch (req.getHttpRequest().protocolVersion()) {
473+
case HTTP_1_0 -> attributes.put("http.flavour", "1.0");
474+
case HTTP_1_1 -> attributes.put("http.flavour", "1.1");
475+
}
476+
477+
tracer.onTraceStarted(threadContext, "rest-" + channel.request().getRequestId(), name, attributes);
478+
}
479+
480+
private void traceException(RestChannel channel, Throwable e) {
481+
this.tracer.onTraceException("rest-" + channel.request().getRequestId(), e);
482+
}
483+
437484
private static void sendContentTypeErrorMessage(@Nullable List<String> contentTypeHeader, RestChannel channel) throws IOException {
438485
final String errorMessage;
439486
if (contentTypeHeader == null) {
@@ -450,6 +497,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
450497
copyRestHeaders(request, threadContext);
451498
validateErrorTrace(request, channel);
452499
} catch (IllegalArgumentException e) {
500+
startTrace(threadContext, channel);
453501
channel.sendResponse(RestResponse.createSimpleErrorResponse(channel, BAD_REQUEST, e.getMessage()));
454502
return;
455503
}
@@ -477,20 +525,19 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
477525
return;
478526
}
479527
} else {
480-
channel.setTracePath(handlers.getPath());
481-
tracer.onTraceStarted(threadContext, channel);
528+
startTrace(threadContext, channel, handlers.getPath());
482529
dispatchRequest(request, channel, handler, threadContext);
483530
return;
484531
}
485532
}
486533
} catch (final IllegalArgumentException e) {
487-
tracer.onTraceStarted(threadContext, channel);
488-
tracer.onTraceException(channel, e);
534+
startTrace(threadContext, channel);
535+
traceException(channel, e);
489536
handleUnsupportedHttpMethod(uri, null, channel, getValidHandlerMethodSet(rawPath), e);
490537
return;
491538
}
492539
// If request has not been handled, fallback to a bad request error.
493-
tracer.onTraceStarted(threadContext, channel);
540+
startTrace(threadContext, channel);
494541
handleBadRequest(uri, requestMethod, channel);
495542
}
496543

@@ -690,31 +737,6 @@ private void close() {
690737
}
691738
inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(-contentLength);
692739
}
693-
694-
@Override
695-
public void setTracePath(String path) {
696-
delegate.setTracePath(path);
697-
}
698-
699-
@Override
700-
public String getTracePath() {
701-
return delegate.getTracePath();
702-
}
703-
704-
@Override
705-
public String getSpanId() {
706-
return delegate.getSpanId();
707-
}
708-
709-
@Override
710-
public String getSpanName() {
711-
return delegate.getSpanName();
712-
}
713-
714-
@Override
715-
public Map<String, Object> getAttributes() {
716-
return delegate.getAttributes();
717-
}
718740
}
719741

720742
private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circuitBreakerService) {

‎server/src/main/java/org/elasticsearch/tasks/Task.java‎

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,17 @@
1111
import org.elasticsearch.action.ActionResponse;
1212
import org.elasticsearch.cluster.node.DiscoveryNode;
1313
import org.elasticsearch.common.io.stream.NamedWriteable;
14-
import org.elasticsearch.tracing.Traceable;
1514
import org.elasticsearch.xcontent.ToXContent;
1615
import org.elasticsearch.xcontent.ToXContentObject;
1716

1817
import java.io.IOException;
19-
import java.util.HashMap;
2018
import java.util.Map;
2119
import java.util.Set;
2220

2321
/**
2422
* Current task information
2523
*/
26-
public class Task implements Traceable {
24+
public class Task {
2725

2826
/**
2927
* The request header to mark tasks with specific ids
@@ -257,26 +255,4 @@ public TaskResult result(DiscoveryNode node, ActionResponse response) throws IOE
257255
throw new IllegalStateException("response has to implement ToXContent to be able to store the results");
258256
}
259257
}
260-
261-
@Override
262-
public String getSpanId() {
263-
return String.valueOf(id);
264-
}
265-
266-
@Override
267-
public String getSpanName() {
268-
return action;
269-
}
270-
271-
@Override
272-
public Map<String, Object> getAttributes() {
273-
274-
TaskId parentTask = getParentTaskId();
275-
Map<String, Object> attributes = new HashMap<>();
276-
attributes.put(Traceable.AttributeKeys.TASK_ID, id);
277-
if (parentTask.isSet()) {
278-
attributes.put(Traceable.AttributeKeys.PARENT_TASK_ID, parentTask.toString());
279-
}
280-
return attributes;
281-
}
282258
}

‎server/src/main/java/org/elasticsearch/tasks/TaskManager.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,12 +145,22 @@ public Task register(String type, String action, TaskAwareRequest request) {
145145
registerCancellableTask(task);
146146
} else {
147147
Task previousTask = tasks.put(task.getId(), task);
148-
tracer.onTraceStarted(threadContext, task);
149148
assert previousTask == null;
149+
startTrace(threadContext, task);
150150
}
151151
return task;
152152
}
153153

154+
private void startTrace(ThreadContext threadContext, Task task) {
155+
TaskId parentTask = task.getParentTaskId();
156+
Map<String, Object> attributes = new HashMap<>();
157+
attributes.put(Tracer.AttributeKeys.TASK_ID, task.getId());
158+
if (parentTask.isSet()) {
159+
attributes.put(Tracer.AttributeKeys.PARENT_TASK_ID, parentTask.toString());
160+
}
161+
tracer.onTraceStarted(threadContext, "task-" + task.getId(), task.getAction(), attributes);
162+
}
163+
154164
public <Request extends ActionRequest, Response extends ActionResponse> Task registerAndExecute(
155165
String type,
156166
TransportAction<Request, Response> action,
@@ -207,7 +217,7 @@ private void registerCancellableTask(Task task) {
207217
CancellableTask cancellableTask = (CancellableTask) task;
208218
CancellableTaskHolder holder = new CancellableTaskHolder(cancellableTask);
209219
cancellableTasks.put(task, holder);
210-
tracer.onTraceStarted(threadPool.getThreadContext(), task);
220+
startTrace(threadPool.getThreadContext(), task);
211221
// Check if this task was banned before we start it. The empty check is used to avoid
212222
// computing the hash code of the parent taskId as most of the time bannedParents is empty.
213223
if (task.getParentTaskId().isSet() && bannedParents.isEmpty() == false) {
@@ -262,7 +272,7 @@ public Task unregister(Task task) {
262272
return removedTask;
263273
}
264274
} finally {
265-
tracer.onTraceStopped(task);
275+
tracer.onTraceStopped("task-" + task.getId());
266276
}
267277
}
268278

0 commit comments

Comments
 (0)