Skip to content
This repository was archived by the owner on Aug 2, 2022. It is now read-only.

Conversation

@tlfeng
Copy link

@tlfeng tlfeng commented Feb 8, 2021

Issue #, if available:
#47

Description of changes:
This PR is inherited from #82 here are the additional changes. I have also marked them in the code.

  • Re-base the code from opendistro-1.1 branch (ES 7.1.1) to opendistro-1.9 branch (ES 7.8.0)
  • Change the parameter name from params to query_params in the JSON string builder in funtion HttpInput:toXContent() to be able to reuse URLInfo component for the custom webhook URL in Kibana plugin
  • Change the URL path in the unit test test monitor HttpInput with non JSON response due to the hidden index change
  • In alerting/build.gradle, add comments for the new configuration of the dependency versions, and remove httpasyncclient from dependency as it has been declared in alerting-core subproject

Minor changes:

  • Change a function name from validateFields to areFieldsValid to because it returns a boolean value
  • Remove duplicate '+' from the error message for the above validateFields() validation function
  • Revert changing the orders of some import classes to reduce file differences of the PR

As the original PR has been merged (but reverted later on: #161, #162), I assume all the changes are reviewed, so I changed the code as less as possible.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tlfeng tlfeng changed the title Add a new input type for monitors - HttpInput (Inherited from PR #82) Adding HTTP Input type for monitors (Inherited from PR #82) Feb 8, 2021
scheme = clusterHosts[clusterIndex].schemeName,
host = clusterHosts[clusterIndex].hostName,
port = clusterHosts[clusterIndex].port,
path = "_cat/plugins",
Copy link
Author

Choose a reason for hiding this comment

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

I changed the path from _cluster/health to _cat/plugins ( Here is the code in the original PR)

const val HOST_FIELD = "host"
const val PORT_FIELD = "port"
const val PATH_FIELD = "path"
const val PARAMS_FIELD = "query_params"
Copy link
Author

@tlfeng tlfeng Feb 8, 2021

Choose a reason for hiding this comment

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

I changed the name of PARAMS_FIELD from "params" to "query_params". (Here is the code in the original PR)

// httpasyncclient 4.1.4 depends on httpcore-nio 4.4.10 which causes the conflict
force "org.apache.httpcomponents:httpcore-nio:4.4.12"
// httpasyncclient 4.1.4 depends on httpclient 4.5.6 which causes the conflict
force "org.apache.httpcomponents:httpclient:${versions.httpclient}"
Copy link
Author

Choose a reason for hiding this comment

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

I changed the version number of some dependencies and add some comments (Here is the code in the original PR)

@tlfeng tlfeng changed the title Adding HTTP Input type for monitors (Inherited from PR #82) Add HTTP Input type for monitors (Inherited from PR #82) Feb 8, 2021
// Verify parameters are valid during creation
init {
require(areFieldsValid()) {
"Either one of url or scheme + host + port + path + params can be set."
Copy link
Author

Choose a reason for hiding this comment

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

I changed the function name from validateFields to areFieldsValid, and removed a duplicate + from the error message. (Here is the code in the original PR)

Base automatically changed from master to main February 9, 2021 20:04
@tlfeng
Copy link
Author

tlfeng commented Mar 18, 2022

The feature is replaced by ClusterMetrics monitor (PR: opensearch-project/alerting#221), and is released in OpenSearch 1.3.0 (Release notes: https://github.com/opensearch-project/opensearch-build/blob/1.3.0/release-notes/opensearch-release-notes-1.3.0.md#opensearch-alerting). 🎉

@tlfeng tlfeng closed this Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants