Improve Task Manager instrumentation by dgieselaar · Pull Request #99160 · elastic/kibana

dgieselaar · 2021-05-04T06:44:49Z

In #90955, I dialed down the cardinality of Task Manager transaction names due to the performance impact of breakdown metrics. Now that some performance issues are fixed in the APM Agent, and the need for better observability of the Task Manager is evident, it makes sense to revert those changes and add additional instrumentation.

The following changes were made:

Disparate transaction types for run, markAvailableTasksAsClaimed, markTaskAsRunning
Use the task type as the transaction name for run transaction types
Wrap task manager/action operations in custom spans, to group ES calls together
Store the traceparent in the task object, to enable distributed tracing for tasks (ie, if a task schedules another task, it will show up in the same trace)

Additionally, I've set refresh to false in some situations, as by default the SO client will use wait_for, and wait on a shard refresh. That could unnecessarily delay task completion, and consume resources that could otherwise be freed up. moved to #99919

For the screenshots, I disabled http/https instrumentation to prevent the http spans from showing up, and I locally worked around an instrumentation issue in the Node.js agent that messes up span relationships (see elastic/apm-agent-nodejs#1964 for a description of the problem).

Before:

After:

This reverts commit 54f848d.

ymao1

Looks good! Left a comment about adding some tests. Do you think you can provide instructions for generating the charts you have in your PR locally? Is that possible? I'd love to see it all working.

x-pack/plugins/task_manager/server/queries/task_claiming.ts

x-pack/plugins/actions/server/lib/action_executor.ts

x-pack/plugins/task_manager/server/task_running/task_runner.ts

gmmorris · 2021-05-24T09:23:42Z

Hi @elastic/kibana-alerting-services

Did anyone discuss the state of this PR with @dgieselaar ?
Will Dario be continuing this when he's back from leave, or are we going to do something with it ourselves? 🤔

ymao1 · 2021-05-24T11:29:20Z

Hi @elastic/kibana-alerting-services

Did anyone discuss the state of this PR with @dgieselaar ?
Will Dario be continuing this when he's back from leave, or are we going to do something with it ourselves? 🤔

Sorry, I dropped the ball on that. I'm happy to add unit tests for the apm calls but I still don't know how to get APM running to see the traces. I can look through the docs

ymao1 · 2021-05-24T19:11:02Z

@elasticmachine merge upstream

…rent

ymao1

LGTM

smith · 2021-05-27T15:09:47Z

@elasticmachine merge upstream

gmmorris · 2021-06-02T10:58:40Z

x-pack/plugins/task_manager/server/task.ts

+  /**
+   * The serialized traceparent string of the current APM transaction or span.
+   */
+  traceparent?: string;
+


I think we should add a note in the Readme.md about using this field to trace the execution of tasks in apm.
Other plugins might want to utilise this.

gmmorris · 2021-06-02T11:01:40Z

x-pack/plugins/task_manager/server/task_scheduling.ts

-    return await this.store.schedule(modifiedTask);
+    return await this.store.schedule({
+      ...modifiedTask,
+      traceparent: agent.currentTraceparent ?? '',


Does an empty string tell APM that there is no trace here? Or is this going to cause all tasks that don't have a traceparent to appear in APM as related because they all get grouped under an "empty string" transaction? 😬

gmmorris · 2021-06-02T11:25:35Z

@elasticmachine merge upstream

gmmorris

LGTM but added a couple of questions.
I'd appreciate if another member of @elastic/kibana-alerting-services took a look as @ymao1 as not just reviewer here but also contributer. :)

chrisronline · 2021-06-02T14:57:46Z

x-pack/plugins/task_manager/server/task_running/task_runner.ts

    try {
      this.task = this.definition.createTaskRunner(modifiedContext);
-      const result = await this.task.run();
+      const result = await withSpan({ name: 'run', type: 'task manager' }, () => this.task!.run());


Just curious, why is this.task! necessary in this PR, but not in master? (on the above line)

chrisronline

LGTM!

Pretty cool to see how useful this integration is and will continue to be for us and users!

ymao1 · 2021-06-04T18:44:33Z

@elasticmachine merge upstream

kibanamachine · 2021-06-04T20:28:28Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`taskManager`	43	44	+1

History

💚 Build #128605 succeeded f8b64e0
💚 Build #127697 succeeded 689abac
💚 Build #127216 succeeded a01134c
💔 Build #127209 failed 0be04d6
💚 Build #126967 succeeded 419a10b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Instrument task manager * Don't refresh after SO updates * Update snapshot test, remove beforeRun instrumentation * Revert "Don't refresh after SO updates" This reverts commit 54f848d. * Fix task_store unit test * Adding tests and updating ConcreteTaskInstance interface with traceparent * Reverting unnecessary changes Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Ying Mao <ying.mao@elastic.co>

kibanamachine · 2021-06-04T20:32:49Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Instrument task manager * Don't refresh after SO updates * Update snapshot test, remove beforeRun instrumentation * Revert "Don't refresh after SO updates" This reverts commit 54f848d. * Fix task_store unit test * Adding tests and updating ConcreteTaskInstance interface with traceparent * Reverting unnecessary changes Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Ying Mao <ying.mao@elastic.co> Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co> Co-authored-by: Ying Mao <ying.mao@elastic.co>

* master: (90 commits) Fix UI breaks on providing long search keyword in 'Search Box' (elastic#101385) Adds css class to EuiDescriptionListDescription in order to break word on exception details card (elastic#101481) [Lens] Increase timings for drag and drop tests (elastic#101380) [Lens] Fix editor react error on configuration panel (elastic#101367) [Fleet] Move integrations to a separate app (elastic#99848) Fix incorrect message displayed on importing Timeline Templates (elastic#101288) [Cases] RBAC (elastic#95058) [APM] Visual improvements for new APM layout with left navigation (elastic#101360) [master] More precise alerts matching (elastic#99820) [Lens] Value in legend (elastic#101353) Revert "[Reporting] ILM policy for managing reporting indices (elastic#100130)" (elastic#101358) [Discover] Fix header row of data grid in Firefox (elastic#101374) Add link to advanced setting in Discover (elastic#101154) Url service locators (elastic#101045) [Timelion] Update the removal message to mention the exact version (elastic#100994) [Security Solution][Detection Engine] Test cases for alias failure test cases where we don't copy aliases correctly (elastic#101437) [Event Log] Adding `type_id` to saved object array in event log (elastic#100939) [Reporting] Add `location.url` info to console message logs (elastic#101427) [Security Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format (elastic#101349) Improve Task Manager instrumentation (elastic#99160) ...

packages/kbn-apm-utils/src/index.ts

dgieselaar added 2 commits May 3, 2021 13:52

Instrument task manager

9b6bb25

Don't refresh after SO updates

54f848d

dgieselaar requested a review from gmmorris May 4, 2021 08:33

dgieselaar added 2 commits May 4, 2021 11:57

Update snapshot test, remove beforeRun instrumentation

4e04a61

Merge branch 'master' of github.com:elastic/kibana into instrument-tm

41df89d

dgieselaar marked this pull request as ready for review May 4, 2021 10:00

dgieselaar requested a review from a team as a code owner May 4, 2021 10:00

dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels May 4, 2021

dgieselaar added 3 commits May 4, 2021 12:06

Merge branch 'master' of github.com:elastic/kibana into instrument-tm

0d8a791

Merge branch 'master' of github.com:elastic/kibana into instrument-tm

5fb04de

Merge branch 'master' of github.com:elastic/kibana into instrument-tm

256a1b0

This was referenced May 6, 2021

Test impact of using refresh: false for task manager internals #99444

Closed

Upgrade elastic.apm.* configuration to "beta" #99328

Closed

dgieselaar added 3 commits May 12, 2021 15:28

Merge branch 'master' of github.com:elastic/kibana into instrument-tm

41065a6

Revert "Don't refresh after SO updates"

bad492c

This reverts commit 54f848d.

Fix task_store unit test

86af743

ymao1 reviewed May 14, 2021

View reviewed changes

x-pack/plugins/task_manager/server/queries/task_claiming.ts Show resolved Hide resolved

x-pack/plugins/actions/server/lib/action_executor.ts Show resolved Hide resolved

x-pack/plugins/task_manager/server/task_running/task_runner.ts Show resolved Hide resolved

kibanamachine and others added 3 commits May 24, 2021 15:11

Merge branch 'master' into instrument-tm

419a10b

Adding tests and updating ConcreteTaskInstance interface with tracepa…

0be04d6

…rent

Reverting unnecessary changes

a01134c

ymao1 approved these changes May 25, 2021

View reviewed changes

ymao1 added Feature:Actions Feature:Task Manager v8.0.0 labels May 25, 2021

Merge branch 'master' into instrument-tm

689abac

smith added the auto-backport Deprecated - use backport:version if exact versions are needed label May 27, 2021

gmmorris reviewed Jun 2, 2021

View reviewed changes

Merge branch 'master' into instrument-tm

f8b64e0

gmmorris approved these changes Jun 2, 2021

View reviewed changes

chrisronline self-requested a review June 2, 2021 11:41

ymao1 added the Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// label Jun 2, 2021

chrisronline reviewed Jun 2, 2021

View reviewed changes

chrisronline approved these changes Jun 2, 2021

View reviewed changes

Merge branch 'master' into instrument-tm

608c322

ymao1 merged commit c79a29a into elastic:master Jun 4, 2021

kibanamachine mentioned this pull request Jun 4, 2021

[7.x] Improve Task Manager instrumentation (#99160) #101434

Merged

gmmorris mentioned this pull request Jun 15, 2021

[RAC][Epic] Observability of the alerting framework phase 1 #98902

Closed

trentm reviewed Jun 23, 2021

View reviewed changes

packages/kbn-apm-utils/src/index.ts Show resolved Hide resolved

Conversation

dgieselaar commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmmorris commented May 24, 2021

Uh oh!

ymao1 commented May 24, 2021

Uh oh!

ymao1 commented May 24, 2021

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

smith commented May 27, 2021

Uh oh!

gmmorris Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

gmmorris Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

gmmorris commented Jun 2, 2021

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

chrisronline Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

chrisronline left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 commented Jun 4, 2021

Uh oh!

kibanamachine commented Jun 4, 2021

💚 Build Succeeded

Metrics [docs]

API count

History

Uh oh!

kibanamachine commented Jun 4, 2021

💚 Backport successful

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dgieselaar commented May 4, 2021 •

edited

Loading