Repository navigation
forgerock: fix endTime query parameter handling - #11409
Merged
Merged
Conversation
efd6
added
bugfix
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime.
Contributor
🚀 Benchmarks reportTo see the full report comment with |
efd6
marked this pull request as ready for review
October 15, 2024 03:11
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
chrisberkhout
approved these changes
Oct 15, 2024
|
Hi @efd6 I see that this PR is aiming to solve issue that I reported. However I noticed one additional thing that I noticed so not sure if this PR is going to solve it or should I create a new issue. When we use "now" as endtime, events that happened few millisecond before it are not immediately exposed in ForgeRock audit logs and they get lost as next request starts with that end date and these logs belong to previous request. My suggestion would be to reduce "now" (when used) by few milliseconds or even seconds. In this case those logs would be picked up with next request. Thanks |
Contributor
Author
|
@igrujic-ap Thanks for the heads up. I will include that in the change here. There are two options:
|
Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosed as a conservative value (the report claimed milliseconds of documents were lost).
💚 Build Succeeded
History
cc @efd6 |
|
|
Thank you @efd6. Do you know release schedule for this fix? We would like to test and let you know if everything is working as expected. |
Contributor
Author
|
@igrujic-ap Just waiting for approval for the most recent additions. |
Contributor
|
Package forgerock - 1.18.4 containing this change is available at https://epr.elastic.co/search?package=forgerock |
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 4, 2025
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime. Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosen as a conservative value (the report claimed milliseconds of documents were lost).
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 5, 2025
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime. Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosen as a conservative value (the report claimed milliseconds of documents were lost).
This file contains hidden or 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
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.
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots