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.

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

[APM] Add default message to alerts. #78930

Merged

Conversation

cauemarcondes
Copy link
Contributor

closes #78573

Screenshot 2020-09-30 at 11 53 18

Screenshot 2020-09-30 at 11 54 42

Screenshot 2020-09-30 at 11 56 53

Screenshot 2020-09-30 at 11 59 05

Screenshot 2020-09-30 at 12 00 10

Screenshot 2020-09-30 at 12 00 53

Screenshot 2020-09-30 at 13 02 49

Screenshot 2020-09-30 at 13 02 32

@cauemarcondes cauemarcondes requested a review from a team as a code owner September 30, 2020 11:12
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@cauemarcondes
Copy link
Contributor Author

@formgeist @bmorelli25 @sqren as suggested, I created a very basic message for each alert type available. I also added the threshold in the Transaction duration anomaly alert.

@sorenlouv
Copy link
Member

We should not output ENVIRONMENT_ALL. You can use getEnvironmentLabel(environment) for this

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

Overall a much-needed improvement by adding these placeholders. I wanted to ask if it's possible to detect one action from the other, and e.g. use the Subject field for the first line?

Screenshot 2020-10-01 at 10 09 11

to something like

Screenshot 2020-10-01 at 11 23 02

Basically, define a placeholder subject as well for the actions that use the subject field?

I think we'll make the messages more consistent across Observability in later design implementation.

@sorenlouv
Copy link
Member

sorenlouv commented Oct 1, 2020

I wanted to ask if it's possible to detect one action from the other, and e.g. use the Subject field for the first line?
@formgeist

Alerting doesn't support default values for anything but the message field yet. But it's something @pmuellr is looking into

Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

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

I'm not sold on alert is firing because of the following conditions:. I think I actually like the original wording better, as I can see configuration applying to all of the bulleted values, but I can only see conditions applying to the last two. I'm not passionate about this though -- I think they're both fine. So LGTM with recommended changes.

@formgeist
Copy link
Contributor

I'm not sold on alert is firing because of the following conditions:. I think I actually like the original wording better, as I can see configuration applying to all of the bulleted values, but I can only see conditions applying to the last two. I'm not passionate about this though -- I think they're both fine. So LGTM with recommended changes.

With many of the alerts being global, I do see service, environment and type as conditions not just configurations. Not sure if this is even something the user will notice anyhow, so happy to leave as is.

@bmorelli25
Copy link
Member

With many of the alerts being global, I do see service, environment and type as conditions not just configurations.

That's a fair point; I'm on board.

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

'xpack.apm.alerts.action_variables.intervalSize',
{
defaultMessage:
'The interval size the alert will use to check for any incident',
Copy link
Member

@sorenlouv sorenlouv Oct 5, 2020

Choose a reason for hiding this comment

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

If somebody doesn't understand the term "interval size" it might be a good idea to use a synonym in the description. WDYT of this?

Suggested change
'The interval size the alert will use to check for any incident',
'The length of the time period where the alert conditions were met',

or perhaps:

Suggested change
'The interval size the alert will use to check for any incident',
'The alert will trigger if the conditions are met within the given interval',

cc @bmorelli25

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
apm 1276 1275 -1

distributable file count

id before after diff
default 47112 47116 +4

page load bundle size

id before after diff
apm 44.3KB 46.2KB +2.0KB

History

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

@cauemarcondes cauemarcondes merged commit ffc1caa into elastic:master Oct 6, 2020
@cauemarcondes cauemarcondes deleted the apm-alerts-default-message branch October 6, 2020 07:18
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Oct 6, 2020
* adding default message

* addressing pr comments

* addressing pr comments

* addressing pr comments

* addressing pr comments

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 6, 2020
* master: (85 commits)
  Refactor attribute service (elastic#78414)
  [APM] Add default message to alerts. (elastic#78930)
  [Discover] Modify columns and sort when switching index pattern (elastic#74336)
  Document ts project references setup (elastic#78586)
  build all ts refs in single kbn:bootstrap (elastic#79438)
  [TSVB] Allow string fields on value count aggregation (elastic#79267)
  [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049)
  [Fleet] Add loading spinner to page headers (elastic#79568)
  [Security Solution][Resolver] Resolver query panel load more (elastic#79160)
  Add type row to monitor detail page. (elastic#79556)
  Remove license refresh from setup (elastic#79518)
  [docker] add reporting fonts (elastic#74806)
  [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558)
  Trim Hash value before validating it (elastic#79545)
  Warn users when security is not configured (elastic#78545)
  update copy styling (elastic#79313)
  Update dependency @elastic/charts to v23.1.1 (elastic#78459)
  Introduce geo-threshold alerts (elastic#76285)
  elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571)
  remove react-intl from kibana and keep it inside only i18n package (elastic#78956)
  ...
cauemarcondes added a commit that referenced this pull request Oct 6, 2020
* adding default message

* addressing pr comments

* addressing pr comments

* addressing pr comments

* addressing pr comments

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

Co-authored-by: Kibana Machine <[email protected]>
@sorenlouv sorenlouv self-assigned this Oct 12, 2020
@sorenlouv
Copy link
Member

✅ This was tested with the kibana_write_user. Default messages showed up for email and slack (not the other actions).

@sorenlouv sorenlouv added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:alerting apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Add default message to alerts.
7 participants