-
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 backend #142164
[ResponseOps][Stack Connectors] Opsgenie backend #142164
Conversation
To make this connector usable in the Kibana UI, you will need to provide all the UI editing aspects of the connector. The existing connector type user interfaces are defined in [`x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types`](../triggers_actions_ui/public/application/components/builtin_action_types). For more information, see the [UI documentation](../triggers_actions_ui/README.md#create-and-register-new-action-type-ui). |
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 this was a whitespace change 🤔
Pinging @elastic/response-ops (Team:ResponseOps) |
@@ -49,6 +49,8 @@ export { | |||
} from './webhook'; | |||
export type { ActionParamsType as WebhookActionParams } from './webhook'; | |||
|
|||
export { getOpsgenieConnectorType, OpsgenieConnectorTypeId } from './opsgenie'; |
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'll add the OpsgenieActionParams
in the next UI PR when it's actually needed externally
|
||
public getResponseErrorMessage(error: AxiosError<ErrorSchema>) { | ||
return `Message: ${ | ||
error.response?.data.errors?.message ?? error.response?.data.message ?? i18n.UNKNOWN_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.
Can we add another fallback to include the error message? For example, error.response?.data.errors?.message ?? error.response?.data.message ?? error.message ?? i18n.UNKNOWN_ERROR
. I am also thinking if we should improve the message thrown by the SubActionConnector
class in the request
method. At the moment we do
const errorMessage = this.getResponseErrorMessage(error);
throw new Error(errorMessage);
Maybe we can include more details like the status code. For example,
const errorMessage = `Status code: ${error.status}. Message: ${this.getResponseErrorMessage(error)}`;
throw new Error(errorMessage);
What do you think? @pmuellr Any information you think we should include in the error message to help us with SDHs?
} | ||
|
||
private createHeaders() { | ||
return { Authorization: `GenieKey ${this.secrets.apiKey}`, 'Content-Type': 'application/json' }; |
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.
The content type header is added automatically by the framework here https://github.com/cnasikas/kibana/blob/cf689fad73f108d47a8e29a0d5afd1baab6f46b6/x-pack/plugins/actions/server/sub_action_framework/sub_action_connector.ts#L83
} | ||
|
||
public async closeAlert(params: CloseAlertParams) { | ||
const fullURL = new URL(`v2/alerts/${params.alias}/close`, this.config.apiUrl); |
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.
Shouldn't we use the concatPathToURL
helper function? The concatPathToURL
can return the URL
object so we can set the searchParams
.
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 good idea.
const validateDetails = (details: Record<string, string>): string | void => { | ||
let totalChars = 0; | ||
|
||
for (const [key, value] of Object.entries(details)) { | ||
totalChars += key.length + value.length; | ||
|
||
if (totalChars > 8000) { | ||
return i18n.translate('xpack.stackConnectors.opsgenie.invalidDetails', { | ||
defaultMessage: 'details field character count exceeds the 8000 limit', | ||
}); | ||
} | ||
} | ||
}; |
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.
According to the docs here we should only validate the values and not the whole JSON string (keys included). The Extra Properties
is the details
. You can test it if you open the Network Tab in your dev tools. The API docs are misleading. I thought the same as you at first 🙂 .
Also, if we had to validate the whole JSON string I think it is better to do
const validateDetails = (details: Record<string, string>): string | void => {
let totalChars = 0;
try {
JSON.stringify(details).length
} catch (e) { // throw error about JSON stringify }
if (totalChars > 8000) {
return i18n.translate('xpack.stackConnectors.opsgenie.invalidDetails', {
defaultMessage: 'details field character count exceeds the 8000 limit',
});
}
};
because we need to validate {
, ,
, and }
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 catch!
.send({ | ||
params: {}, | ||
}) | ||
.then((resp: 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.
nit: For readability maybe is better to do:
const res = await supertest.post(...).set(...).send(...).expect(200)
// the expects
expect(resp.body.connector_id).to.eql(opsgenieActionId);
....
}); | ||
}); | ||
}); | ||
}); |
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 we should add more tests to validate some of the optional fields.
}); | ||
|
||
it('calls request with the correct arguments for creating an alert', async () => { | ||
const connectorSpy = jest |
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 is better to mock axios
instead of the method of the class. This way if the sub-action framework introduces an unintended breaking change or a feature change your tests can catch it. You can follow the examples here x-pack/plugins/actions/server/sub_action_framework/sub_action_connector.test.ts
public getResponseErrorMessage(error: AxiosError<ErrorSchema>) { | ||
return `Message: ${ | ||
error.response?.data.errors?.message ?? error.response?.data.message ?? i18n.UNKNOWN_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.
I think we should omit the dot. I got an error while testing and I noticed that there are two dots in the message. For example: "Message: To perform this action, use an API key from an API integration.."
If I am reading "Connector collaboration" doc correctly, Opsgenie should go under the |
@cnasikas what do you think? We might not support cases initially. From that doc it seems like any connector that has support for cases should be moved to the |
schema.literal('schedule'), | ||
]); | ||
|
||
const validateDetails = (details: Record<string, string>): string | void => { |
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 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.
Ooooooh got it.
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.
We decided to remove the validation offline because opsgenie will just truncate the strings and not throw an error.
@elasticmachine merge upstream |
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.
Code LTGM! Tested without any issues. I could create and close an alert in OpsGenie. 🚀
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
This PR implements the backend of the opsgenie connector.
Issue: #142776
Opsgenie API Docs: https://docs.opsgenie.com/docs/alert-api#create-alert
Examples
Creating alerts
Postman creating connector
Postman creating an alert
Created Alerts within Opsgenie
Closing alerts
Postman closing an alert
Closed alert within Opsgenie
Testing
Integrations
on the left, click that, then clickAdd Integration
API Integration
Save Integration
Creating a connector
curl
Raw
Closing an alert
curl
Raw