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

Watcher: Ensure mail message ids are unique per watch action #30112

Merged
merged 2 commits into from
Apr 27, 2018
Merged

Watcher: Ensure mail message ids are unique per watch action #30112

merged 2 commits into from
Apr 27, 2018

Conversation

spinscale
Copy link
Contributor

@spinscale spinscale commented Apr 25, 2018 •
edited
Loading

Email message IDs are supposed to be unique. In order to guarantee this,
we need to take the action id of a watch action into account as well,
not just the watch id from the watch execution context. This prevents
that two actions from the same watch execution end up with the same
message id.

Note: I intend to backport this down to 6.3 - please veto as part of the review!

@spinscale
Watcher: Ensure mail message ids are unique per watch action
7bff522 
Email message IDs are supposed to be unique. In order to guarantee this,
we need to take the action id of a watch action into account as well,
not just the watch id from the watch execution context. This prevents
that two actions from the same watch execution end up with the same
message id.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

danielmitterdorfer
danielmitterdorfer approved these changes Apr 26, 2018
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

LGTM.

W.r.t. to backporting: I do not want to block the backport to 6.3 necessarily but I am not sure this bug is critical enough that it is warranted backporting to 6.3 it at this point.

@spinscale spinscale merged commit 707ba28 into elastic:master Apr 27, 2018
spinscale added a commit that referenced this pull request Apr 27, 2018
@spinscale
Watcher: Ensure mail message ids are unique per watch action (#30112)
6fed2d4 
Email message IDs are supposed to be unique. In order to guarantee this,
we need to take the action id of a watch action into account as well,
not just the watch id from the watch execution context. This prevents
that two actions from the same watch execution end up with the same
message id.
@ypid-geberit
Copy link

ypid-geberit commented Apr 27, 2018 •
edited
Loading

Thanks @spinscale!

Closes: https://discuss.elastic.co/t/bug-report-x-pack-watcher-email-action-uses-the-same-message-id-if-multiple-mail-actions-are-used/128399
Ref: #29563

I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended run_test.py to inject Python code after the watch definition is read. Not ideal, but it is maintainable.

dnhatn added a commit that referenced this pull request Apr 27, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
b7727c6 
* 6.x:
  Watcher: Ensure mail message ids are unique per watch action (#30112)
  SQL: Correct error message (#30138)
  SQL: Add BinaryMathProcessor to named writeables list (#30127)
  Tests: Use buildDir as base for generated-resources (#30191)
  Fix SliceBuilderTests#testRandom failures
  Fix edge cases in CompositeKeyExtractorTests (#30175)
  Document time unit limitations for date histograms (#30177)
  Remove licenses missed by the migration
  [DOCS] Updates docker installation package details (#30110)
  Fix TermsSetQueryBuilder.doEquals() method (#29629)
  [Monitoring] Remove unhelpful Monitoring tests (#30144)
  [Test] Fix RenameProcessorTests.testRenameExistingFieldNullValue() (#29655)
  [test] include oss tar in packaging tests (#30155)
  TEST: Update settings should go through cluster state (#29682)
  Add additional shards routing info in ShardSearchRequest (#29533)
  Reinstate missing documentation (#28781)
  Clarify documentation of scroll_id (#29424)
  Fixes Eclipse build for sql jdbc project (#30114)
  Watcher: Fold two smoke test projects into smoke-test-watcher (#30137)
dnhatn added a commit that referenced this pull request Apr 27, 2018
@dnhatn
Merge branch 'master' into ccr
112b5f1 
* master: (24 commits)
  Watcher: Ensure mail message ids are unique per watch action (#30112)
  REST: Remove GET support for clear cache indices (#29525)
  SQL: Correct error message (#30138)
  Require acknowledgement to start_trial license (#30135)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (#30181)
  SQL: Add BinaryMathProcessor to named writeables list (#30127)
  Tests: Use buildDir as base for generated-resources (#30191)
  Fix SliceBuilderTests#testRandom failures
  Build: Fix deb version to use tilde with prerelease versions (#29000)
  Fix edge cases in CompositeKeyExtractorTests (#30175)
  Document time unit limitations for date histograms (#30177)
  Add support for field capabilities to the high-level REST client. (#29664)
  Remove licenses missed by the migration (#30128)
  [DOCS] Updates docker installation package details (#30110)
  Fix TermsSetQueryBuilder.doEquals() method (#29629)
  [Monitoring] Remove unhelpful Monitoring tests (#30144)
  [Test] Fix RenameProcessorTests.testRenameExistingFieldNullValue() (#29655)
  add copyright/scope configuration for intellij to Contributing Guide (#29688)
  [test] include oss tar in packaging tests (#30155)
  TEST: Update settings should go through cluster state (#29682)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 27, 2018
@jasontedor
Merge branch 'master' into thread-pool-info-migration
d03e812 
* master: (7173 commits)
  Bump changelog version to 6.4 (elastic#30217)
  [DOCS] Adds native realm security settings (elastic#30186)
  Test: Switch painless test to 1 shard
  CCS: Drop http address from remote cluster info (elastic#29568)
  Reindex: Fold "from old" tests into reindex module (elastic#30142)
  Convert FieldCapabilitiesResponse to a ToXContentObject. (elastic#30182)
  [DOCS] Added 'on a single shard' to description of max_thread_count. Closes 28518 (elastic#29686)
  [TEST] Redirect links to new locations (elastic#30179)
  Move repository-s3 fixture tests to QA test project (elastic#29372)
  Fail snapshot operations early on repository corruption (elastic#30140)
  Docs: Document `failures` on reindex and friends
  Build global ordinals terms bucket from matching ordinals (elastic#30166)
  Watcher: Ensure mail message ids are unique per watch action (elastic#30112)
  REST: Remove GET support for clear cache indices (elastic#29525)
  SQL: Correct error message (elastic#30138)
  Require acknowledgement to start_trial license (elastic#30135)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (elastic#30181)
  SQL: Add BinaryMathProcessor to named writeables list (elastic#30127)
  Tests: Use buildDir as base for generated-resources (elastic#30191)
  Fix SliceBuilderTests#testRandom failures
  ...
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Implement --no-execute-watch needed for deployment
fcc5d2f 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Support to inject Python code, useful for deployment
f6f6689 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Implement --no-test-index needed for deployment
f208272 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Implement --no-execute-watch needed for deployment
0f7c3ad 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Support to inject Python code, useful for deployment
dd26f1e 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Oct 12, 2018
@ypid-geberit
run_test.py: Implement --no-test-index needed for deployment
18e4377 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
@immon immon mentioned this pull request Feb 19, 2020
Closed
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Jun 25, 2020
@ypid-geberit
run_test.py: Implement --no-execute-watch needed for deployment
fa043dd 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Jun 25, 2020
@ypid-geberit
run_test.py: Support to inject Python code, useful for deployment
db887a4 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
ypid-geberit added a commit to ypid-geberit/examples that referenced this pull request Jun 25, 2020
@ypid-geberit
run_test.py: Implement --no-test-index needed for deployment
19d2f41 
Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.
DanRoscigno pushed a commit to elastic/examples that referenced this pull request Jul 23, 2021
@ypid-geberit
Fix/improve watcher test scripts: YAML support, metadata-git-commit, …
dbc75a9 
…better error handling, --cacert, multiple time fields (#239)

* run_test.py: Improve/cleanup and add YAML support for input files

* Cleanup shell scripts according to ShellCheck recommendations

* run_test.py: Cleanup Indices after test to not pollute tests env

* run_test.py: More useful error message if logging action did not run

* run_test.py: Refactor

* run_test.py: Use load_file() for ES scripts as well to support YAML

* run_test.py: Implement --no-execute-watch needed for deployment

Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.

* run_test.py: Support to inject Python code, useful for deployment

Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.

* run_test.py: Implement --no-test-index needed for deployment

Needed for: elastic/elasticsearch#30112 (comment)

> I already have a workaround in place for this which consists of automatically deploying as many watches as I need to send different mails. Those watches are derived from my watch definition. For this, I extended [run_test.py](https://github.com/elastic/examples/blob/master/Alerting/Sample%20Watches/run_test.py) to inject Python code after the watch definition is read. Not ideal, but it is maintainable.

* run_test.py: Add --metadata-git-commit switch to augment watch metadata

* run_test.py: Add --cacert parameter

* run_test.py: More useful error message if logging action did not run

* run_test.py: Use `git rev-parse --short HEAD` for --metadata-git-commit

* run_test.py: More useful error message if transform failed

* run_test.py: Implement --minify-scripts

Workaround for: elastic/elasticsearch#35184

* "Scripts may be no longer than 16384 characters." is in ES<v6.6 not >6.6

* run_test.py: Improve compatibility with ES 7.0.x and index templates

* run_test.py: Better error message if expected_response is not defined

* run_test.py: Show watch exception on execution failure

* run_test.py: In case a transform fails the transform input is relevant

* run_test.py: Support multiple time fields

Useful when you have two time fields that in reality should be very
close so in testing it is enough to set them to the same value.

* [run_test.py] ES 7 support. Update to Py3 and drop elasticsearch_xpack.

* [run_test.py] Add --verbose parameter to debug ES responses

* [run_test.py] Comply with Python Enhancement Proposals

* [run_test.py] Comply with reuse.software

* [run_test.py] Avoid `not` in condition to make it easier to understand

* [run_test.py] Use str.format instead of "%s" % for consistency

* [run_test.py] Fix ./run_all_tests.sh test run. All passing again.

* [run_test.py] Support nested fields in time_fields test parameter

Example:

```yaml
time_fields:
  - '@timestamp'
  - 'event.created'
```

* [run_test.py] Use dict.get shortcut
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmitterdorfer danielmitterdorfer danielmitterdorfer approved these changes

Assignees
No one assigned
Labels
>bug :Data Management/Watcher v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spinscale @elasticmachine @ypid-geberit @danielmitterdorfer @colings86 @bleskes