Adds telemetry support to alerting and actions plugins #49832 #54214
Conversation
…lemetry-kpi-beta # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
YulNaumenko
added
Feature:Alerting
v8.0.0
release_note:skip
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
…lemetry-kpi-beta # Conflicts: # x-pack/legacy/plugins/alerting/server/plugin.ts
|
title for this PR should be more descriptive: "adds telemetry support to alerting and actions plugins" |
YulNaumenko
changed the title
There was a problem hiding this comment.
I think the way this is written it will work correctly (or close enough) even with a set of Kibana servers, but it seems very expensive as we'll be making multiple SO calls for every execution just to update the telemetry counter. Shouldn't these be batched and the SO updated every couple of minutes, instead of when the action executions happen?
But I don't know. Maybe this is the way all the other plugins work, and the overhead with the SO calls isn't that bad?
| }); | ||
| } | ||
|
|
||
| export async function incrementActionExecutionsCount( |
There was a problem hiding this comment.
It seems very expensive to make this call on every action execution, as it's making 3 SO calls in the function. Is there a pattern where incrementActionExecutionsCount() would just update a local object in the Kibana server, and then every 30 minutes or so update the SO? Not sure how other plugins do this.
| let executionsByActionTypes = {}; | ||
|
|
||
| try { | ||
| const { attributes } = (await savedObjectsClient.get( |
There was a problem hiding this comment.
It seems like there's a race condition here - shouldn't storeActionsTelemetry() use optimistic concurrency to update the SO? It doesn't seem like it does. Maybe for telemetry we don't care that much if it's off by a little?
|
@elasticmachine merge upstream |
…lemetry-kpi-beta # Conflicts: # x-pack/legacy/plugins/alerting/server/task_runner/task_runner.ts
…lemetry-kpi-beta # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
|
Since we're past feature freeze for v7.6.0 this PR should be bumped to v7.7.0 |
Summary
#49832
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This includes a feature addition or change that requires a release note and was labeled appropriately