Skip to content
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

[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry. #88532

Merged
merged 9 commits into from
Jan 27, 2021
Merged

[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry. #88532

merged 9 commits into from
Jan 27, 2021

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jan 15, 2021 •
edited
Loading

  1. Fixed replace '.' only if it is a first symbol of alertTypeId or actionTypeId.
{
stack_stats.kibana.plugins.alerts.count_active_by_type.xpack.uptime.alerts.monitorStatus: 1, 
stack_stats.kibana.plugins.alerts.count_active_by_type.__index-threshold: 1,
}

and

{ 
stack_stats.kibana.plugins.actions.count_active_by_type.__server-log: 1 
}
  1. Added missing logic for count_active_by_type in the actions telemetry. New flow is to fetch all unique connectorIds, which are in usage in 'references.type': 'action' and then group this number by actionTypeId from the savedObject internalRepository bulkGet request by the selected connectorIds.
    Example of the actions response part in stats API request:
    https://localhost:5601/tgx/api/stats?extended=true
"actions": {
            "runs": 128,
            "count_total": 5,
            "count_by_type": {
                "slack": 1,
                "teams": 1,
                "server_log": 2,
                "index": 1
            },
            "count_active_total": 2,
            "count_active_by_type": {
                "slack": 1,
                "server_log": 1
            }
        },

@YulNaumenko YulNaumenko self-assigned this Jan 15, 2021
@YulNaumenko YulNaumenko linked an issue Jan 16, 2021 that may be closed by this pull request
[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry. #88123
Closed
@YulNaumenko YulNaumenko added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Telemetry bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 labels Jan 22, 2021
@YulNaumenko YulNaumenko marked this pull request as ready for review January 22, 2021 22:05
@YulNaumenko YulNaumenko requested a review from a team as a code owner January 22, 2021 22:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

pmuellr
pmuellr reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Left a comment about creating a single function to do the . -> __ mapping.

Also, I'm curious why we changed this so that only the first . is changed. Was there a problem with other ones, eg, with xpack.uptime.alerts.monitorStatus? I'm not sure how our data is ingested into the telemetry indices, but will having an "internal ." be a problem for those property names, for how telemetry might query for them?

x-pack/plugins/actions/server/usage/actions_telemetry.ts Outdated
const actions = await actionsBulkGet(bulkFilter);
const countByType = actions.saved_objects.reduce(
(actionTypeCount: Record<string, number>, action) => {
const hasFirstSymbolDot = action.attributes.actionTypeId.startsWith('.');
Copy link
Member

Choose a reason for hiding this comment

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

Since this code is the same as on line 65, I think we should reorganize it into a single function in this module, that's called in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why we need this replacement of the first symbol '.' is from this issue

"reason" : "object field starting or ending with a [.] makes object resolution ambiguous: [.webhook]"
"reason" : "object field starting or ending with a [.] makes object resolution ambiguous: [.index-threshold]"
"reason" : "object field starting or ending with a [.] makes object resolution ambiguous: [.server-log]"

and commented here. I agree that we should have a single method for doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we should add handling for end symbol '.' as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no first symbol '.', easier to keep the original alertTypeId and actionTypeId which are easier to identify. So I think we should keep only replacements which are required.

Copy link
Member

Choose a reason for hiding this comment

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

cool, thx.

I'd guess we won't see any types that end with ., but I guess it could cause problems if we are doing "text/keyword" multi-fields (eg, where you would append .keyword to the field), so probably makes sense to process a trailing . as well. Just in case :-)

@YulNaumenko YulNaumenko requested a review from pmuellr January 26, 2021 05:44
@gmmorris
Copy link
Contributor

@elasticmachine merge upstream

ymao1
ymao1 approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Just a nit about tests.

x-pack/plugins/actions/server/usage/actions_telemetry.test.ts Outdated
const mockEsClient = jest.fn();
mockEsClient.mockReturnValue({
aggregations: {
byActionTypeId: {
value: {
types: { '.index': 1, '.server-log': 1 },
types: { '.index': 1, '.server-log': 1, 'some.type': 1 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add another type that ends with a . to test that it gets stripped?

x-pack/plugins/alerts/server/usage/alerts_telemetry.test.ts Outdated
const mockEsClient = jest.fn();
mockEsClient.mockReturnValue({
aggregations: {
byAlertTypeId: {
value: {
types: { '.index-threshold': 2 },
types: { '.index-threshold': 2, 'logs.alert.document.count': 1 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above...should we add a type that ends in a . to test that it gets stripped?

@YulNaumenko
Copy link
Contributor Author

jenkins retest this please

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

pmuellr
pmuellr approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@YulNaumenko YulNaumenko merged commit 4663345 into elastic:master Jan 27, 2021
@YulNaumenko YulNaumenko mentioned this pull request Jan 27, 2021
Merged
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jan 27, 2021
@YulNaumenko @kibanamachine
[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and aler…
0544525 
…t types telemetry. (elastic#88532)

* [Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry.

* fixed count_active_by_type for actions

* fixed tests

* Fixed due to comments.

* Fixed due to comments.

Co-authored-by: Kibana Machine <[email protected]>
YulNaumenko added a commit that referenced this pull request Jan 27, 2021
@YulNaumenko @kibanamachine
[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and aler…
2043589 
…t types telemetry. (#88532) (#89448)

* [Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry.

* fixed count_active_by_type for actions

* fixed tests

* Fixed due to comments.

* Fixed due to comments.

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@gmmorris gmmorris mentioned this pull request Dec 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ymao1 ymao1 ymao1 approved these changes

@pmuellr pmuellr pmuellr approved these changes

Assignees

@YulNaumenko YulNaumenko

Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Alerts][Actions][Telemetry] Fix mappings for Kibana actions and alert types telemetry.
6 participants
@YulNaumenko @elasticmachine @gmmorris @kibanamachine @pmuellr @ymao1