Skip to content

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

Merged
dimitris-athanasiou merged 2 commits intoelastic:feature/apm-integrationfrom
dimitris-athanasiou:parent-spans-of-nested-tasks
Nov 17, 2021
Merged

Capture task span context in thread context to parent nested tasks#80758
dimitris-athanasiou merged 2 commits intoelastic:feature/apm-integrationfrom
dimitris-athanasiou:parent-spans-of-nested-tasks

Conversation

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor

@dimitris-athanasiou dimitris-athanasiou commented Nov 16, 2021

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

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

elasticmachine commented Nov 16, 2021

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

@dimitris-athanasiou dimitris-athanasiou added >non-issue and removed Team:Distributed Meta label for distributed team. 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
Copy link
Copy Markdown
Contributor

@idegtiarenko idegtiarenko Nov 16, 2021

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

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

@AthenaEryma AthenaEryma 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
Copy Markdown
Contributor

idegtiarenko commented Nov 17, 2021

@elasticmachine please run elasticsearch-ci/part-1

@idegtiarenko
Copy link
Copy Markdown
Contributor

idegtiarenko commented Nov 17, 2021

@elasticmachine please run elasticsearch-ci/part-2

DaveCTurner
DaveCTurner reviewed Nov 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@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
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Member

@tlrx tlrx Nov 17, 2021

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
Copy Markdown
Contributor Author

@dimitris-athanasiou dimitris-athanasiou Nov 17, 2021

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
Copy Markdown
Member

@tlrx tlrx Nov 17, 2021

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
Copy Markdown
Member

@DaveCTurner DaveCTurner Nov 17, 2021

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

+1 more reviewer

@AthenaEryma AthenaEryma AthenaEryma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Distributed/Distributed A catch all label for anything in the Distributed 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 @AthenaEryma @DaveCTurner