- Notifications
You must be signed in to change notification settings - Fork 519
[o365] Simplification of data fetching logic #16024
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
| Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
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.
Because we lean on repeated input loops more now, I think it may be worth doing the dropping of the retry events in the agent with a beat processor, rather than sending the event to be dropped by the ingest pipeline. An example of this is here corresponding to this alternative way of saying {"retry": true}.
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.
Sure. Done.
3645b09 to d0e4387 Compare 🚀 Benchmarks reportTo see the full report comment with |
| @@ -1,4 +1,5 @@ | |||
| interval: {{interval}} | |||
| max_executions: 10000 | |||
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.
On the basis of a support case, we may want to make this configurable, but defaulting to a higher value like this.
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.
👍 Done.
f2827ac to 8df8cae Compare e56dd5c to 43ccbd7 Compare | @efd6 I split the mock server part into a separate draft PR. |
💚 Build Succeeded
History
|
| Package o365 - 3.1.0 containing this change is available at https://epr.elastic.co/package/o365/3.1.0/ |
Proposed commit message
Notes for the reviewer
I started this before the last 4 PRs, and I checked that there are no conflicts with those changes:
We no longer parse the next page URLs, since we have the
endTimefrom the initial request.o365: fix error propagation within cel program #15445
This error handling logic isn't relevant in the new flattened structure.
The new code still limits subscription requests, but to a lesser degree, which I think is better overall.
The new CEL code passes with the original system tests.
The system tests have been updated to use a mock o365 server rather than thestreamtool. The mock server has configurability, assertions and logging beyond what is used in the system test, which may be useful for future debugging. The amount of code is not insignificant, and the quality is just okay, so if this is a maintenance concern, it can be moved to a separate PR or removed entirely. Let me know what you think.Introduction of a new mock server has been moved to a separate PR: #16190
Checklist
changelog.ymlfile.How to test this PR locally
You can manually run against the new mock server.
Get the
o365mock.gofile from #16190, then start the mock server like this:In another terminal, run the CEL code in
mitolike this:Stop the mock server with Ctrl+C to trigger it's shutdown report.
You can remove the
mitoauth configuration if you disableCheckAccessTokenin the mock server's inline configuration.This version of
mitowill run faster than the following one, which rate limits to 1 rps:Related issues