-
Notifications
You must be signed in to change notification settings - Fork 100
Switch to the new _fleet/_fleet_search and _fleet/_fleet_msearch Elasticsearch Fleet APIs, remove holes detection and refreshes #814
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
Conversation
…ticsearch Fleet APIs, remove holes detection and refreshes * Switch to the new _fleet/_fleet_msearch and _fleet/_fleet_search Fleet APIs endpoints for the searches that required refreshes and wait for checkpoints. The new API handles refreshes and checkpoints waits. * Separate queues for _msearch and _fleet_msearch, to avoid delays on searches without checkpoints wait. Use _fleet/_fleet_msearch endpoint if search is requested with wait_for_checkpoints. Use _fleet/_fleet_search for the monitor hits fetch. * Had to copy over the search and msearch wrappers from go-elasticsearch library and customize them for _fleet_search and _fleet_msearch. These could be removed once the library is updated for these new endpoints. * Removed the holes detection and refresh op code as it's not longer used.
|
This pull request does not have a backport label. Could you fix it @aleksmaus? 🙏
NOTE: |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
This pull request does not have a backport label. Could you fix it @aleksmaus? 🙏
NOTE: |
scunningham
left a comment
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.
Less code than before. Nice!
| return err | ||
| } | ||
|
|
||
| buf.WriteString("{") |
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.
Added extra blit for the normal cases where there's only one index.
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.
with additional param in there it's less branches to cover for balancing the brackets, it's easier to understand this way. let me know if still want to optimize this blit away.
| buf.WriteString("{ }\n") | ||
| } else { | ||
| buf.WriteString(`{"index": "`) | ||
| buf.WriteString(`"index": "`) |
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.
Changed logic for the case where index = "". Should fail on empty index name then.
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.
updated
internal/pkg/bulk/opSearch.go
Outdated
| if len(checkpoints) > 0 { | ||
| buf.WriteString(`, "wait_for_checkpoints": `) | ||
| // Write array as string, example: [1,2,3] | ||
| buf.WriteString(strings.ReplaceAll(fmt.Sprint([]int64(checkpoints)), " ", ",")) |
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.
doesn't happen enough to optimize, but curious if there's a faster way than two passes.
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.
simple loop is faster
BenchmarkTwoPhases/0-16 9670161 122.5 ns/op 26 B/op 2 allocs/op
BenchmarkTwoPhases/1-16 5927617 203.5 ns/op 40 B/op 3 allocs/op
BenchmarkTwoPhases/2-16 3636922 331.2 ns/op 56 B/op 5 allocs/op
BenchmarkTwoPhases/3-16 2711920 442.8 ns/op 64 B/op 6 allocs/op
BenchmarkTwoPhases/4-16 2291353 566.9 ns/op 88 B/op 7 allocs/op
BenchmarkTwoPhases/5-16 1854615 684.2 ns/op 96 B/op 8 allocs/op
BenchmarkTwoPhases/6-16 1544910 702.6 ns/op 104 B/op 9 allocs/op
BenchmarkTwoPhases/7-16 1469547 822.2 ns/op 112 B/op 10 allocs/op
BenchmarkTwoPhases/8-16 1314674 905.5 ns/op 136 B/op 11 allocs/op
BenchmarkTwoPhases/9-16 1000000 1019 ns/op 144 B/op 12 allocs/op
BenchmarkLoop/0-16 758645307 1.548 ns/op 0 B/op 0 allocs/op
BenchmarkLoop/1-16 51346444 23.28 ns/op 4 B/op 1 allocs/op
BenchmarkLoop/2-16 41733303 28.96 ns/op 8 B/op 1 allocs/op
BenchmarkLoop/3-16 35340536 34.36 ns/op 8 B/op 1 allocs/op
BenchmarkLoop/4-16 26870481 45.16 ns/op 16 B/op 1 allocs/op
BenchmarkLoop/5-16 24264476 51.31 ns/op 16 B/op 1 allocs/op
BenchmarkLoop/6-16 21013568 56.16 ns/op 16 B/op 1 allocs/op
BenchmarkLoop/7-16 18831969 64.75 ns/op 16 B/op 1 allocs/op
BenchmarkLoop/8-16 15635251 72.55 ns/op 24 B/op 1 allocs/op
BenchmarkLoop/9-16 14547670 78.62 ns/op 24 B/op 1 allocs/op
will update
| ) | ||
|
|
||
| if queue.ty == kQueueFleetSearch { | ||
| req := es.FleetMsearchRequest{ |
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.
Add comment here about it being temporary.
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.
added comment
|
Nice to see this made it into Elasticsearch. Its a pity that currently we need to have all the elasticsearch client code in here. Could you follow with the client team and open an issue to get this added to the client so we can remove it later again? On the ES PR I see a 7.16 label but not sure if this made it into 7.16? If yes, should we also backport it? |
I advised against back porting it because we are past feature freeze, and this only adds risk without providing a ton of upside. As for the Elasticsearch code, this is, in theory, temporary. This logic should role into the msearch API once it gets past "experimental". I think we should defer adding it to the Elastic Client until we are convinced that it will not make it into the mainstream for whatever reason. |
I agree with @scunningham on this. Since the API is experimental, customized for fleet, and we possibly will keep iterating on this, the client library will be always be behind. So we will end up switching between using the client library and removing the copied over code and copying over the code again to customize it for the latest changes in the API. |
…ticsearch Fleet APIs, remove holes detection and refreshes (#814) * Switch to the new _fleet/_fleet_search and _fleet/_fleet_msearch Elasticsearch Fleet APIs, remove holes detection and refreshes * Switch to the new _fleet/_fleet_msearch and _fleet/_fleet_search Fleet APIs endpoints for the searches that required refreshes and wait for checkpoints. The new API handles refreshes and checkpoints waits. * Separate queues for _msearch and _fleet_msearch, to avoid delays on searches without checkpoints wait. Use _fleet/_fleet_msearch endpoint if search is requested with wait_for_checkpoints. Use _fleet/_fleet_search for the monitor hits fetch. * Had to copy over the search and msearch wrappers from go-elasticsearch library and customize them for _fleet_search and _fleet_msearch. These could be removed once the library is updated for these new endpoints. * Removed the holes detection and refresh op code as it's not longer used. (cherry picked from commit a2fb073)
|
@aleksmaus This change is not in 8.0 but only master. Is my assumption correct that this should also be in the 8.0 branch? Here is the automatic backport, please merge if you agree: #863 |
…ticsearch Fleet APIs, remove holes detection and refreshes (#814) * Switch to the new _fleet/_fleet_search and _fleet/_fleet_msearch Elasticsearch Fleet APIs, remove holes detection and refreshes * Switch to the new _fleet/_fleet_msearch and _fleet/_fleet_search Fleet APIs endpoints for the searches that required refreshes and wait for checkpoints. The new API handles refreshes and checkpoints waits. * Separate queues for _msearch and _fleet_msearch, to avoid delays on searches without checkpoints wait. Use _fleet/_fleet_msearch endpoint if search is requested with wait_for_checkpoints. Use _fleet/_fleet_search for the monitor hits fetch. * Had to copy over the search and msearch wrappers from go-elasticsearch library and customize them for _fleet_search and _fleet_msearch. These could be removed once the library is updated for these new endpoints. * Removed the holes detection and refresh op code as it's not longer used. (cherry picked from commit a2fb073)
…ticsearch Fleet APIs, remove holes detection and refreshes (#814) (#863) * Switch to the new _fleet/_fleet_search and _fleet/_fleet_msearch Elasticsearch Fleet APIs, remove holes detection and refreshes * Switch to the new _fleet/_fleet_msearch and _fleet/_fleet_search Fleet APIs endpoints for the searches that required refreshes and wait for checkpoints. The new API handles refreshes and checkpoints waits. * Separate queues for _msearch and _fleet_msearch, to avoid delays on searches without checkpoints wait. Use _fleet/_fleet_msearch endpoint if search is requested with wait_for_checkpoints. Use _fleet/_fleet_search for the monitor hits fetch. * Had to copy over the search and msearch wrappers from go-elasticsearch library and customize them for _fleet_search and _fleet_msearch. These could be removed once the library is updated for these new endpoints. * Removed the holes detection and refresh op code as it's not longer used. (cherry picked from commit a2fb073) Co-authored-by: Aleksandr Maus <[email protected]>
What is the problem this PR solves?
Replaces the holes detections, refresh and refetch for the seq_no based index changes monitoring and queries with the new custom Elasticsearch Fleet APIs that handle refreshes and wait for checkpoints under the hood for us.
How does this PR solve the problem?
endpoints for the searches that required refreshes and wait for
checkpoints. The new API handles refreshes and checkpoints waits.
searches without checkpoints wait. Use _fleet/_fleet_msearch endpoint if search is requested with
wait_for_checkpoints. Use _fleet/_fleet_search for the monitor hits
fetch.
library and customize them for _fleet_search and _fleet_msearch.
These could be removed once the library is updated for these new
endpoints.
used.
How to test this PR locally
Full regression testing specifically in the areas that dispatch actions and the policy changes.
Checklist
Related issues
Wireshark captures for the new requests