-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ResponseOps][Stack Connectors] Opsgenie UI phase 2 #143480
[ResponseOps][Stack Connectors] Opsgenie UI phase 2 #143480
Conversation
Co-authored-by: nastasha-solomon <[email protected]>
Co-authored-by: nastasha-solomon <[email protected]>
Co-authored-by: nastasha-solomon <[email protected]>
Co-authored-by: nastasha-solomon <[email protected]>
Co-authored-by: nastasha-solomon <[email protected]>
Co-authored-by: nastasha-solomon <[email protected]>
…ana into opsgenie-ui-phase-2
...ck/plugins/stack_connectors/public/connector_types/stack/opsgenie/create_alert/schem.test.ts
Outdated
Show resolved
Hide resolved
expect(() => | ||
decodeCreateAlert({ | ||
message: 'hi', | ||
responders: [{ name: 'sam', type: 'team', invalidField: 'scott' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if it would be useful to show these additional fields somehow if they are populated in the JSON in the non-JSON editor view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah ideally we'd have that. We didn't think we'd be able to create those components in time so we opted for the json editor create the complex arrays. We could include them in the future though.
x-pack/plugins/stack_connectors/public/connector_types/stack/opsgenie/params.tsx
Show resolved
Hide resolved
TextFieldWithMessageVariables, | ||
} from '@kbn/triggers-actions-ui-plugin/public'; | ||
import { EuiFormRow, EuiSelect, RecursivePartial } from '@elastic/eui'; | ||
import { ActionParamsProps, ActionConnectorMode } from '@kbn/triggers-actions-ui-plugin/public'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a rule, if I select the OpsGenie action and just click Save
without entering any values, it does not save but also does not show any validation errors on the form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, working on a fix.
Opsgenie connectors have the following configuration properties. | ||
|
||
Name:: The name of the connector. The name is used to identify a connector in the management UI connector listing, or in the connector list when configuring an action. | ||
URL:: The Opsgenie URL: https://api.opsgenie.com. For the EU instance of Opsgenie use: https://api.eu.opsgenie.com. If you are using the <<action-settings, `xpack.actions.allowedHosts`>> setting, make sure the hostname is added to the allowed hosts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO since this URL is not something we control, we could just provide these as example URLs. Something like this:
URL:: The Opsgenie URL: https://api.opsgenie.com. For the EU instance of Opsgenie use: https://api.eu.opsgenie.com. If you are using the <<action-settings, `xpack.actions.allowedHosts`>> setting, make sure the hostname is added to the allowed hosts. | |
URL:: The Opsgenie URL. For example, https://api.opsgenie.com or https://api.eu.opsgenie.com. If you are using the <<action-settings,`xpack.actions.allowedHosts`>> setting, make sure the hostname is added to the allowed hosts. |
.../plugins/stack_connectors/public/connector_types/stack/opsgenie/create_alert/translations.ts
Outdated
Show resolved
Hide resolved
[[opsgenie-action-create-alert-configuration]] | ||
===== Configure the create alert action | ||
|
||
You can configure the create alert action through the form view or using a JSON editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I know this page is following the example of the example connector pages, IMO rather than listing each field and meaning here, it's more important to (1) have the appropriate API documentation up-to-date with these properties and descriptions, and (2) have tooltips in the UI for any fields that aren't obvious (e.g. "alias", "entity", "source"). If you want me to add those tooltips in this PR, @jonathan-buttner let me know. Otherwise, I'll create a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point @lcawl . Let's do a follow up PR to address that. I'll add these fields in the docs here and we can remove them once we have the tooltips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested generating some alerts in Opsgenie using a variety of the 'create alert' API parameters, in test mode and by using the connector in a rule, and overall LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
import { CloseAlert } from './close_alert'; | ||
import userEvent from '@testing-library/user-event'; | ||
|
||
describe('CreateAlert', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: close alert
import { isEmpty, isObject } from 'lodash'; | ||
import * as i18n from './translations'; | ||
|
||
const MessageNonEmptyString = new rt.Type<string, string, unknown>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like magic to me 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha I actually grabbed it from here: https://github.com/elastic/kibana/blob/main/packages/kbn-securitysolution-io-ts-types/src/non_empty_string/index.ts
and modified it a bit :)
if (error.response?.data?.errors != null) { | ||
const message = this.getDetailedErrorMessage(error.response?.data?.errors); | ||
if (!isEmpty(message)) { | ||
return `${mainMessage}: ${message}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if message
is undefined
will the message be what ever in the main message: undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lodash isEmpty
will actually return true
if message
is undefined
so the if block won't be executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly me, hahaha
* "username": "must be a well-formed email address" | ||
* } | ||
*/ | ||
errors: schema.maybe(schema.any()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think changing it to Record<string, string>
will be risky? If the error schema is different it will throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was my concern, I figured the safest route is just to JSON.stringify
the result and not actually make assumptions on how it is formatted. When I looked through the Opsgenie API docs I didn't see any references to the format of the errors
object. So my understanding of the format is just based on my experience with trying to cause errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that make sense!
<I18nProvider> | ||
<TestConnectorForm | ||
connector={connector} | ||
executeEnabled={true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we test the case where executeEnabled=false
or undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might be missing what you're getting at here. The executeEnabled
doesn't have any affect on the field I added (executionMode
) right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry wrong line of code. I mean the executionMode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, yep I'll add some tests for those scenarios 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't think there's much else I can test here since the TestConnectorForm
is passing ActionConnectorMode.Test
always right? Or am I missing something? I'll add a check to ensure the other text field shouldn't be displayed when executionMode === Test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! Ensuring that the other text field shouldn't be displayed is enough.
@@ -173,6 +177,7 @@ export interface ActionParamsProps<TParams> { | |||
isLoading?: boolean; | |||
isDisabled?: boolean; | |||
showEmailSubjectAndMessage?: boolean; | |||
executionMode?: ActionConnectorMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better if the executionMode
is required and the framework will pass either ActionConnectorMode.Test
or ActionConnectorMode.NORMAL (or similar)
to avoid confusion in the future in case we need to add more execution modes. Another option is to change the executionMode
to testingMode?: boolean
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll switch it to being required and create a new enum for the ActionForm code path 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one place where the email connector is instantiated outside of the action framework: x-pack/plugins/synthetics/public/legacy_uptime/components/settings/default_email.tsx
After chatting with @ymao1 it seemed easiest to keep executionMode
as optional that way that instance doesn't need to pass a value for no reason. I still changed the location in the action form to pass in the ActionForm
enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
|
||
if (isEnabled && isDisplayed) { | ||
await flyOutCancelButton.click(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait for the flyout to disappear and throw if not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job Jonathan! Code LGTM and tested without any issues 🚀. I noticed that you cannot update (add more keys) the details
attribute. This is how OpsGenie behaves. I tested their API. Btw, I created a flaky test runner here: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1512
|
||
await testSubjects.missingOrFail('messageInput'); | ||
await retry.waitFor('message input to be displayed', async () => { | ||
await testSubjects.selectValue('opsgenie-subActionSelect', 'createAlert'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we need to select the value on each retry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was seeing flakiness with the form changing so I think reselecting the value seemed to help.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Issue: #142776
This PR implements most of the Phase 2 section of the above issue. We decided not to implement the ability to dynamically adding the fields. Instead we are going to provide a JSON editor that users can easily define those complex fields (e.g.
responders
,visibleTo
).Notable details:
io-ts
on the frontend andkbn/config-schema
on the backendkbn/config-schema
on the frontend I was told that Joi is not supported by all browsers and would increase the page load size by a lotTesting
Refer to this PR's testing section for details on how to create an Opsgenie environment: #142164
Retrieving an alert after it is created/closed
If you'd like to retrieve an alert via Opsgenie's API you can use curl or Postman and use the following requests
Using the Tiny ID
Grab the tiny value (shown with the
#
sign under the alert message header in Opsgenie, i.e. 22 in the image below)Alias
Use the alias specified in the request. The docs state that you can't retrieve a closed alert using the alias though.
ID
This is used by default, it is the
id
field in the alert body.Notes
I'm not able to get the
responders
andvisibleTo
fields to cause any noticeable changes within the alert when it is created. I believe this is because the plan you are give when trying Opsgenie for free is theEssential
plan.https://support.atlassian.com/opsgenie/docs/create-a-default-api-integration/
The Essential plan only allows you to add integrations to individual teams. The Opsgenie docs say if the API key used to create an alert is tied to a team, then
responders
will be overridden to the team and you can't change it. I assume this is also affecting thevisibleTo
field.https://docs.opsgenie.com/docs/alert-api#create-alert
Valid JSON examples
JSON examples
More examples can be found here: https://docs.opsgenie.com/docs/alert-api#create-alert
Examples
Opsgenie Icon
Create alert rule action form
JSON editor
Close alert rule action form