Skip to content

Capture task span context in thread context to parent nested tasks #80758

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

Jump to bottom
Merged
merged 2 commits into from
Nov 17, 2021
Merged

Capture task span context in thread context to parent nested tasks #80758

merged 2 commits into from
Nov 17, 2021

Conversation

dimitris-athanasiou
Copy link
Contributor

Pass a task's span context via the ThreadContext to parent nested tasks.

@dimitris-athanasiou dimitris-athanasiou added WIP :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. and removed WIP labels Nov 16, 2021
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Nov 16, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@dimitris-athanasiou dimitris-athanasiou added >non-issue and removed Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. labels Nov 16, 2021
idegtiarenko
idegtiarenko reviewed Nov 16, 2021
View reviewed changes
x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java Outdated
@@ -81,7 +131,7 @@ public void onTaskUnregistered(Task task) {

public static class CapturingSpanExporter implements SpanExporter {

private List<SpanData> capturedSpans = new ArrayList<>();
private List<SpanData> capturedSpans = Collections.synchronizedList(new ArrayList<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

This was populated in small chunks from multiple threads and loosing updates sometimes

gwbrown
gwbrown approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not being at all familiar with the OpenTelemetry API, but having reviewed all the relevant PRs up until this point: LGTM. (Once CI is happy)

@idegtiarenko
Copy link
Contributor

@elasticmachine please run elasticsearch-ci/part-1

@idegtiarenko
Copy link
Contributor

@elasticmachine please run elasticsearch-ci/part-2

DaveCTurner
DaveCTurner reviewed Nov 17, 2021
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Hopefully CI will be happy with this, LGTM at least.

@dimitris-athanasiou dimitris-athanasiou force-pushed the parent-spans-of-nested-tasks branch from 1dd8e9e to 516d0e5 Compare November 17, 2021 10:45
tlrx
tlrx approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java
* {@link StoredContext}.
* @param headersToRemove the request headers to remove
*/
public StoredContext removeRequestHeaders(Set<String> headersToRemove) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd call this stashWithoutHeader()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the other stashXXX() methods start off with the default thread context, I avoided prefixing this one with stash. I don't feel strongly on it though. What do you reckon @DaveCTurner ?

Copy link
Member

Choose a reason for hiding this comment

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

I see, feel free to merge like it is so that we don't waste time on naming :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I also don't want to say stash. I didn't come up with an obviously better name, although maybe I'd have called it something like withoutRequestHeaders or removingRequestHeaders rather than using the imperative remove. NBD anyway, naming is hard.

@dimitris-athanasiou dimitris-athanasiou force-pushed the parent-spans-of-nested-tasks branch from 13875ee to 886255c Compare November 17, 2021 12:22
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 17, 2021
@DaveCTurner
Use utils from elastic#80758
4a7dd14
@dimitris-athanasiou dimitris-athanasiou merged commit 2f51e5f into elastic:feature/apm-integration Nov 17, 2021
@dimitris-athanasiou dimitris-athanasiou deleted the parent-spans-of-nested-tasks branch November 17, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlrx tlrx tlrx approved these changes

@idegtiarenko idegtiarenko idegtiarenko approved these changes

@DaveCTurner DaveCTurner DaveCTurner left review comments

@gwbrown gwbrown gwbrown approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >non-issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dimitris-athanasiou @elasticmachine @idegtiarenko @tlrx @gwbrown @DaveCTurner