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

[APM] Scope APM alert creation to environment #65681

Merged
merged 8 commits into from
May 13, 2020
Merged

[APM] Scope APM alert creation to environment #65681

merged 8 commits into from
May 13, 2020

Conversation

cauemarcondes
Copy link
Contributor

closes #65349

Screenshot 2020-05-07 at 14 53 52

Screenshot 2020-05-07 at 14 54 23

@cauemarcondes cauemarcondes requested a review from a team as a code owner May 7, 2020 13:20
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label May 7, 2020
@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
Contributor Author

retest

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

sorenlouv
sorenlouv approved these changes May 11, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm - but maybe @dgieselaar wants a final word?

@dgieselaar
Copy link
Member

@cauemarcondes @sqren we can't use hooks (at least not out of the box) if we want to support creating/editing alerts outside of the APM app. I'm not sure if we should add hooks here because it will make supporting those use cases harder. Do we have an alternative that doesn't add any technical debt?

@cauemarcondes
Copy link
Contributor Author

@cauemarcondes @sqren we can't use hooks (at least not out of the box) if we want to support creating/editing alerts outside of the APM app. I'm not sure if we should add hooks here because it will make supporting those use cases harder. Do we have an alternative that doesn't add any technical debt?

@dgieselaar there is already a bug that will fix that #65399, but it's gonna be released only on 7.9. I don't think it's gonna be an easy fix to solve this issue.

@sorenlouv
Copy link
Member

sorenlouv commented May 11, 2020 •
edited
Loading

@cauemarcondes @sqren we can't use hooks (at least not out of the box) if we want to support creating/editing alerts outside of the APM app. I'm not sure if we should add hooks here because it will make supporting those use cases harder. Do we have an alternative that doesn't add any technical debt?

Would it fix the problem if we wrapped the component in the appropriate context providers ?

@dgieselaar
Copy link
Member

@cauemarcondes Editing for 7.8 will be disabled, but we will need to fix it for 7.8. Adding a hook will/could make that harder depending on the fix we come up with. So my question is whether this is the only and the right way to fix it given that it will add technical debt that we will need to pay off later.

@sqren could be, and would be my preferred fix, but I'm not sure if that it is feasible.

@cauemarcondes
Copy link
Contributor Author

@dgieselaar @sqren wasn't it what @smith wanted to fix here #65412.

@cauemarcondes cauemarcondes requested a review from dgieselaar May 11, 2020 16:08
@cauemarcondes cauemarcondes requested a review from dgieselaar May 12, 2020 08:35
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cauemarcondes cauemarcondes merged commit 9a13779 into elastic:master May 13, 2020
@cauemarcondes cauemarcondes deleted the environment-alert branch May 13, 2020 08:38
@cauemarcondes cauemarcondes mentioned this pull request May 13, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request May 13, 2020
@cauemarcondes @elasticmachine
[APM] Scope APM alert creation to environment (elastic#65681)
f811cf4 
* adding environment to alerting

* fixing translations

* addressing pr comments

* addressing PR comments

* addressing PR comments

Co-authored-by: Elastic Machine <[email protected]>
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request May 13, 2020
@cauemarcondes @elasticmachine
[APM] Scope APM alert creation to environment (elastic#65681)
8b14985 
* adding environment to alerting

* fixing translations

* addressing pr comments

* addressing PR comments

* addressing PR comments

Co-authored-by: Elastic Machine <[email protected]>
@cauemarcondes cauemarcondes mentioned this pull request May 13, 2020
Merged
cauemarcondes added a commit that referenced this pull request May 13, 2020
@cauemarcondes @elasticmachine
[APM] Scope APM alert creation to environment (#65681) (#66361)
29dcaaf 
* adding environment to alerting

* fixing translations

* addressing pr comments

* addressing PR comments

* addressing PR comments

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

Co-authored-by: Elastic Machine <[email protected]>
cauemarcondes added a commit that referenced this pull request May 13, 2020
@cauemarcondes @elasticmachine
[APM] Scope APM alert creation to environment (#65681) (#66360)
fcc230e 
* adding environment to alerting

* fixing translations

* addressing pr comments

* addressing PR comments

* addressing PR comments

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

Co-authored-by: Elastic Machine <[email protected]>
v1v added a commit to v1v/kibana that referenced this pull request May 13, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/e2e-changes
58bdbbd 
* upstream/master: (223 commits)
  [Ingest] Support root level yaml variables in agent stream template (elastic#66120)
  [Snapshot Restore] Fix error when deleting snapshots behind reverse proxy (elastic#66147)
  [Lens] fix empty state for pie (elastic#66206)
  [APM] Improve e2e tests (elastic#66373)
  [ML] Data Frame Analytics: Fix steps to be named phases. (elastic#65855)
  [Discover] Encode context link filter part (elastic#63831)
  [APM] Scope APM alert creation to environment (elastic#65681)
  Move Kibana Usage collectors outside the telemetry plugin (elastic#65663)
  [ML] Data Frame Analytics: Fix confusion matrix data grid width. (elastic#65818)
  Switch to core application service (elastic#63443)
  Removes use of prefer_v2_templates (elastic#66316)
  [Maps] handle case where fit to bounds does not match any documents (elastic#66307)
  log error instead of throw (elastic#66254)
  [plugin-helpers] remove outdated postinstall task (elastic#66324)
  Spaces - migrate default space and enter space view to KP (elastic#66098)
  [APM] Change plugin id for `apm_oss` to `apmOss` (elastic#66164)
  [Maps] convert map_selectors to TS (elastic#65905)
  [uptime/usage-collector] add missing await (elastic#66079)
  [Ingest] Add additional attributes to the Datasources Saved Object (elastic#66127)
  [Endpoint]EMT-339: add new policy response schema (elastic#66264)
  ...
@dgieselaar dgieselaar self-assigned this May 14, 2020
@dgieselaar
Copy link
Member

This is working correctly. There's no way to use the environment as a variable in the template (like you can with service.name and transaction.type) but we can address that in #66712.

@dgieselaar dgieselaar added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 15, 2020
@dgieselaar dgieselaar removed their assignment May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

@dgieselaar dgieselaar dgieselaar approved these changes

Assignees
No one assigned
Labels
apm:test-plan-7.8.0 apm:test-plan-done Pull request that was successfully tested during the test plan release_note:fix Team:APM All issues that need APM UI Team support v7.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scope APM alert creation to environment
5 participants
@cauemarcondes @elasticmachine @dgieselaar @sorenlouv @kibanamachine