sampling: fix pubsub implementation #5126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
axw
force-pushed
the
sampling-pubsub-checkpoints
branch
from
589df06
to
c670d64
Compare
axw
force-pushed
the
sampling-pubsub-checkpoints
branch
from
c670d64
to
bf62e28
Compare
The initial implementation was written as a ~quick hack, with the expectation that it would be replaced by the Changes API. It was broken due to its ignorance of data streams, and multi-shard indices. Sequence numbers are only comparable within a single shard. Given that there is no known delivery date for the Changes API, we propose to instead revise the pubsub implementation to address the problems by: - enforcing single-shard indices for sampled trace data streams - searching (now single-shard) backing indices individually In addition, we now use global checkpoints to bound searches, and use PIT (point in time) for paging through results. Querying underlying indices and global checkpoints requires an additional "monitor" index privilege.
axw
force-pushed
the
sampling-pubsub-checkpoints
branch
from
bf62e28
to
845e77e
Compare
Simplify by just using direct searches with a rnage on _seq_no, using the most recently observed _seq_no value as the lower bound. We can do this within the loop as well (i.e. until there are no more results, or we've observed the global checkpoint.)
Refresh indices after observing an updated global checkpoint to ensure document visibility is correct up to the observed global checkpoint.
|
This pull request is now in conflicts. Could you fix it @axw? 🙏 |
There was a problem hiding this comment.
Everything looks reasonable, but this is enough out of my realm of knowledge for me to not be able to provide a definitive 👍 / 👎 . I'd be happy to listen to a quick walkthrough of the code, or merge and deal with any potential issues that arise since the current implementation is broken.
|
This pull request is now in conflicts. Could you fix it @axw? 🙏 |
simitt
reviewed
May 25, 2021
simitt
approved these changes
May 26, 2021
mergify bot
pushed a commit
that referenced
this pull request
May 26, 2021
* sampling: fix pubsub implementation The initial implementation was written as a ~quick hack, with the expectation that it would be replaced by the Changes API. It was broken due to its ignorance of data streams, and multi-shard indices. Sequence numbers are only comparable within a single shard. Given that there is no known delivery date for the Changes API, we propose to instead revise the pubsub implementation to address the problems by: - enforcing single-shard indices for sampled trace data streams - searching (now single-shard) backing indices individually In addition, we now use global checkpoints to bound searches, and use PIT (point in time) for paging through results. Querying underlying indices and global checkpoints requires an additional "monitor" index privilege. * sampling/pubsub: remove PIT again Simplify by just using direct searches with a rnage on _seq_no, using the most recently observed _seq_no value as the lower bound. We can do this within the loop as well (i.e. until there are no more results, or we've observed the global checkpoint.) * sampling/pubsub: only query get metric from _stats * pubsub: force-refresh indices Refresh indices after observing an updated global checkpoint to ensure document visibility is correct up to the observed global checkpoint. * Update changelog * systemtest: fix spurious test failure (cherry picked from commit 94e3201) # Conflicts: # changelogs/head.asciidoc
axw
added a commit
that referenced
this pull request
May 26, 2021
* sampling: fix pubsub implementation (#5126) * sampling: fix pubsub implementation The initial implementation was written as a ~quick hack, with the expectation that it would be replaced by the Changes API. It was broken due to its ignorance of data streams, and multi-shard indices. Sequence numbers are only comparable within a single shard. Given that there is no known delivery date for the Changes API, we propose to instead revise the pubsub implementation to address the problems by: - enforcing single-shard indices for sampled trace data streams - searching (now single-shard) backing indices individually In addition, we now use global checkpoints to bound searches, and use PIT (point in time) for paging through results. Querying underlying indices and global checkpoints requires an additional "monitor" index privilege. * sampling/pubsub: remove PIT again Simplify by just using direct searches with a rnage on _seq_no, using the most recently observed _seq_no value as the lower bound. We can do this within the loop as well (i.e. until there are no more results, or we've observed the global checkpoint.) * sampling/pubsub: only query get metric from _stats * pubsub: force-refresh indices Refresh indices after observing an updated global checkpoint to ensure document visibility is correct up to the observed global checkpoint. * Update changelog * systemtest: fix spurious test failure (cherry picked from commit 94e3201) # Conflicts: # changelogs/head.asciidoc * Delete head.asciidoc Co-authored-by: Andrew Wilkins <axw@elastic.co>
mergify bot
pushed a commit
that referenced
this pull request
Jul 8, 2021
* sampling: fix pubsub implementation The initial implementation was written as a ~quick hack, with the expectation that it would be replaced by the Changes API. It was broken due to its ignorance of data streams, and multi-shard indices. Sequence numbers are only comparable within a single shard. Given that there is no known delivery date for the Changes API, we propose to instead revise the pubsub implementation to address the problems by: - enforcing single-shard indices for sampled trace data streams - searching (now single-shard) backing indices individually In addition, we now use global checkpoints to bound searches, and use PIT (point in time) for paging through results. Querying underlying indices and global checkpoints requires an additional "monitor" index privilege. * sampling/pubsub: remove PIT again Simplify by just using direct searches with a rnage on _seq_no, using the most recently observed _seq_no value as the lower bound. We can do this within the loop as well (i.e. until there are no more results, or we've observed the global checkpoint.) * sampling/pubsub: only query get metric from _stats * pubsub: force-refresh indices Refresh indices after observing an updated global checkpoint to ensure document visibility is correct up to the observed global checkpoint. * Update changelog * systemtest: fix spurious test failure (cherry picked from commit 94e3201) # Conflicts: # apmpackage/apm/0.2.0/data_stream/sampled_traces/manifest.yml # changelogs/head.asciidoc # x-pack/apm-server/sampling/pubsub/pubsub.go # x-pack/apm-server/sampling/pubsub/pubsub_test.go # x-pack/apm-server/sampling/pubsub/pubsubtest/client.go
|
confirmed with SNAPSHOT |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation/summary
The initial implementation was written as a ~quick hack, with the expectation that it would be replaced by the Changes API.
It was broken due to its ignorance of data streams, and multi-shard indices. Sequence numbers are only comparable within a single shard.
Instead of waiting for the Changes API we could take the same approach as elastic/fleet-server#200, using
a new Elasticsearch API built for Fleet Server. The main issue here is that the API does not (currently) support data streams.
Given the above and that there is no known delivery date for the Changes API, we propose to instead revise the pubsub implementation to address the problems by:
In addition, we now use global checkpoints to bound searches (ensuring all replicas have committed the documents). Querying underlying indices and global checkpoints will require an additional "monitor" index privilege.
Checklist
- [ ] Documentation has been updatedHow to test these changes
Related issues
Closes #5119