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

[Logs UI] [Alerting] Alerts management page enhancements #64654

Merged
merged 7 commits into from
May 4, 2020

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Apr 28, 2020

Summary

This PR:

  • Moves us over to using the new log source configuration hook from [Logs UI] Reimplement log source configuration routes in plain HTTP+JSON #64021.
  • Ensures that fetch is an injected dependency, which also kills off several legacy_singleton uses.
  • Ultimately ensures that log alerts can be added, and edited, from the alerts management page. Without relying on anything specific to rendering within the logs app.

Testing

⚠️ There is currently an issue whereby the alert types are registered in mount, instead of setup, to reduce bundle sizes which breaks editing on the alerts management page. If someone starts reviewing this before this is sorted with Ops you can simply move the line this.registerLogsAlertType(pluginsSetup); from mount to setup

Alerting requires SSL, with a local setup this can be enabled like so:

  • yarn es snapshot --ssl
  • yarn start --ssl
  • ssl.verification_mode: none inside of the Filebeat config.

Things to check:

  • Adding an alert works from within logs
  • Adding and editing an alert works from the alerts management page
  • Nothing source configuration related has broken

@Kerry350 Kerry350 requested a review from a team April 29, 2020 12:08
@Kerry350 Kerry350 added Feature:Alerting Feature:Logs UI Logs UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0 v8.0.0 labels Apr 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Kerry350 Kerry350 marked this pull request as ready for review April 29, 2020 12:09
@Kerry350
Copy link
Contributor Author

@elasticmachine merge upstream

@afgomez afgomez self-requested a review May 4, 2020 08:15
Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Pulled the code. I couldn't find anything not working 🎉

import { FormattedMessage } from '@kbn/i18n/react';
import {
ForLastExpression,
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
} from '../../../../../../triggers_actions_ui/public/common';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { IErrorObject } from '../../../../../../triggers_actions_ui/public/types';
import { useSource } from '../../../../containers/source';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this path might be restricted? Maybe it's a bug and it shouldn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to follow this up with the Alerting team. There's a few types we're (logs and metrics) using that aren't exported from public or server, and so aren't really supposed to be used. Once they're bumped up to being part of the public or server contract, I'll remove the lint overrides. I'll handle in a followup 👍

@Kerry350
Copy link
Contributor Author

Kerry350 commented May 4, 2020

@elasticmachine merge upstream

@Kerry350
Copy link
Contributor Author

Kerry350 commented May 4, 2020

@afgomez Thanks for the review 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@Kerry350 Kerry350 merged commit 5bcf2c8 into elastic:master May 4, 2020
Kerry350 added a commit to Kerry350/kibana that referenced this pull request May 4, 2020
* Ensure adding / editing log alerts works from the alerts management page
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 4, 2020
…ana into alerting/np-tests-migration

* 'alerting/np-tests-migration' of github.com:gmmorris/kibana:
  [APM] Agent remote config: validation for Java agent configs (elastic#63956)
  [APM] Fix duplicate index patterns (elastic#64883)
  Drilldowns (elastic#61219)
  [Alerting] fix labels and links in PagerDuty action ui and docs (elastic#64032)
  [Event Log] Ensure sorting tests are less flaky (elastic#64781)
  update endpoint to restrict removing with datasources (elastic#64978)
  [Logs UI] [Alerting] Alerts management page enhancements (elastic#64654)
  Adjust kibana app owning files (elastic#65064)
  Migrate tutorial resources (elastic#64298)
  [Logs UI] Tweak copy in log alerts dialog (elastic#64645)
  [Logs UI] [Alerting] Documentation (elastic#64886)
  [Logs UI] Add dataset filter to ML module setup screen (elastic#64470)
  [TSVB] Fixing memory leak (elastic#64918)
  Bump backport to 5.4.1 (elastic#65041)
Kerry350 added a commit that referenced this pull request May 4, 2020
…5070)

* Ensure adding / editing log alerts works from the alerts management page

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting Feature:Logs UI Logs UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants