Skip to content

[integrations][trend_micro_vision_one] - Add configurable "additional look-back" time option to all data streams - #12382

Merged
ShourieG merged 0 commit into
elastic:mainfrom
ShourieG:enhancement/vision_one
Feb 3, 2025
Merged

[integrations][trend_micro_vision_one] - Add configurable "additional look-back" time option to all data streams#12382
ShourieG merged 0 commit into
elastic:mainfrom
ShourieG:enhancement/vision_one

Conversation

@ShourieG

@ShourieG ShourieG commented Jan 17, 2025

Copy link
Copy Markdown
Contributor

Type of change

  • Enhancement

Proposed commit message

There was a scenario where users observed gaps in ingested data and on further investigation it was found that events fetched were skewed at beginning of the time interval which lead to the conclusion that the source api had some delay in populating the events in the response. With the introduction of this new additional look-back, users can configure it to a value that suits their scenario and avoid gaps in data. By default this is set to '0s' for the traditional behaviour.

NOTE:

Even though the issue was reported for the "detections" data stream, this PR adds this to all the data streams as a safety net to avoid future issues which might be similar, since we cannot say for sure that this issue won't occur for the rest.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@ShourieG
ShourieG requested a review from a team as a code owner January 17, 2025 10:35
@ShourieG ShourieG self-assigned this Jan 17, 2025
@ShourieG ShourieG added integration Label used for meta issues tracking each integration Integration:trend_micro_vision_one TrendAI Vision One enhancement New feature or request Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] labels Jan 17, 2025
@elasticmachine

Copy link
Copy Markdown

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@chrisberkhout chrisberkhout left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For all endpoints:

I think for getting non-initial pages we should just use the nextLink and not add other parameters. This should avoid gaps.

The last page in a sequence will have an empty or partial page. If it doesn't return a nextLink value, we can reuse the current value after the interval. If there is some problem with that, we can use the most recent updatedDateTime seen in results.

We probably never need to set endDateTime. Or, if it's required by the API, it could be set to 1 minute before the current time, which may also help avoiding gaps.

It's not changed here, but I think it's a bug that we set orderBy=updatedDateTime asc and dateTimeTarget=createdDateTime. I think the server will do something like:

SELECT *
FROM data AS d
WHERE d.createdDateTime > '2024-01-01'
ORDER BY d.updatedDateTime ASC

This will not include items updated after our cursor time and may result in choosing the wrong cursor time in following requests.

@ShourieG

Copy link
Copy Markdown
Contributor Author

I think for getting non-initial pages we should just use the nextLink and not add other parameters. This should avoid gaps.

@chrisberkhout We are already using next link for pagination, but it seems that sometimes the events come in late at the source end when the next page is called with a start/end time, and our pagination does not pick up those events within that time window.

@chrisberkhout

Copy link
Copy Markdown
Contributor

I think for getting non-initial pages we should just use the nextLink and not add other parameters. This should avoid gaps.

@chrisberkhout We are already using next link for pagination, but it seems that sometimes the events come in late at the source end when the next page is called with a start/end time, and our pagination does not pick up those events within that time window.

Currently when we use a nextLink, we still override the startDateTime, to the time we sent the last request. That can explain skipped data. If we use startDateTime to the time of the most recent data we've seen, that should close the gaps, assuming new updates become visible in the order of their timestamps (probably true if it's a single database). Even better, we can avoid setting startDateTime on non-initial requests and nextLink should give us the first thing after what we've seen. It also avoid getting data more times than we need to.

Setting endDateTime to be a little bit earlier than the current time is extra protection, and would work if new updates become visible out of order (e.g. there are multiple databases, or the update time is set outside of the database).

@ShourieG

ShourieG commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

I think for getting non-initial pages we should just use the nextLink and not add other parameters. This should avoid gaps.

@chrisberkhout We are already using next link for pagination, but it seems that sometimes the events come in late at the source end when the next page is called with a start/end time, and our pagination does not pick up those events within that time window.

Currently when we use a nextLink, we still override the startDateTime, to the time we sent the last request. That can explain skipped data. If we use startDateTime to the time of the most recent data we've seen, that should close the gaps, assuming new updates become visible in the order of their timestamps (probably true if it's a single database). Even better, we can avoid setting startDateTime on non-initial requests and nextLink should give us the first thing after what we've seen. It also avoid getting data more times than we need to.

Setting endDateTime to be a little bit earlier than the current time is extra protection, and would work if new updates become visible out of order (e.g. there are multiple databases, or the update time is set outside of the database).

@chrisberkhout, If you look at the detections agent code, we are only using startDateTime on the initial request, then for all successive paginations we use the nextLink until pagination ends. This pagination link completely overrides the current url value to what was sent from the source, so highly unlikely this would be faulty. It is only in the successive interval when we use the value of endDateTime for the next start duration, and it seems to be the case that there's delay in populating the data at the source and because of this the pagination ends before all the data is actually fetched for that interval, so when the next one begins we are left with a gap in between these 2 intervals. This change was to address that gap, since we cant use a nextLink for the beginning of the 1st request in an interval.

@chrisberkhout

Copy link
Copy Markdown
Contributor

Currently when we use a nextLink, we still override the startDateTime

Sorry, I had this backwards. We do still set startDateTime for the pagination request, but it's cleared when response.pagination sets url.value.

@chrisberkhout, If you look at the detections agent code, we are only using startDateTime on the initial request, then for all successive paginations we use the nextLink until pagination ends. This pagination link completely overrides the current url value to what was sent from the source, so highly unlikely this would be faulty.

That's right. I thought we would still get startDateTime set, because the request.transforms do run again for each pagination request, before the response.pagination transforms. But I checked with the system test, and setting url.value in response.pagination does override/clear the url.params. In the end we have the header settings from request.transforms but the URL/params settings are only the nextLink ones, set by reponse.pagination.

So that's fine.


It is only in the successive interval when we use the value of endDateTime for the next start duration, and it seems to be the case that there's delay in populating the data at the source and because of this the pagination ends before all the data is actually fetched for that interval, so when the next one begins we are left with a gap in between these 2 intervals. This change was to address that gap, since we cant use a nextLink for the beginning of the 1st request in an interval.

Re-using the last nextLink value for a new sequence is an option, but maybe not the best option.

The existing logic asks: what's new since my last request?
The PR logic asks: what's new since X seconds before my last request? (default X=0)
My suggested logic asks: what's new since the last data I saw?

We want to close the gap, but with a lookback we generate more duplicates than necessary. Why not just ask to continue from where we left off?


In the alert data stream, we should match orderBy and dateTimeTarget for performance reasons and to avoid some (more obscure) cases of missed data.

@ShourieG

ShourieG commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

@chrisberkhout, So there seems to be some api limitations here:
detections api: https://automation.trendmicro.com/xdr/api-v3/#tag/Search/paths/~1v3.0~1search~1detections/get

  1. There is no orderBy clause for this API so we are at the mercy of the source to provide ordered data, otherwise we can't use the last_event.eventTimeDT reliably and have to rely on last_response.url.params.Get "endDateTime"
  2. The timestamp eventTimeDT in the response can be used but if it comes in unsorted it won't be possible to do so, and I couldn't find any docs confirming that the data is sorted.

So the suggestion -

My suggested logic asks: what's new since the last data I saw?

would only work if the data is sorted or if we migrate to CEL and sort by eventTimeDT, but I don't see an option here with what we currently have.

@cmarlettalivi

Copy link
Copy Markdown

Hi, I also reported this problem and found that:

  1. The events returned by the API are sorted in decreasing order of time and can include events with eventTimeDT equal to the two search extremes (startDate and endDate)
  2. subsequent readings should restart with startDate: eventTimeDT of the last event stored in the previous reading session (5 minutes before, since interval: 5m in configuration). This can result in the same event being read twice.
  3. It would be necessary to provide for the management of a bookmark in the readings in the sessions, taking care to verify that the events read with eventTimeDT=startDate have not already been stored (for example, you can compare the uuid value of the bookmark event with that of the events read subsequently)
@chrisberkhout

chrisberkhout commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

Thanks for the observations @cmarlettalivi! We do have some logic in the ingest pipeline to set an _id based on the UUID and some other fields. An attempt to index a document with the same _id value will be rejected (as long as its the same backing index). So that mostly avoids the potential duplicates.

Hey @ShourieG, that's a really good point about the order for detections. I don't want to hold this up, so I'll share my final thoughts and if anything is unclear we can discuss, but otherwise feel free to go ahead with what you think is best.

It sounds like detections may in fact be ordered by event time. We do have first_event as well as last_event. So I think there are 3 potential strategies for starting a new sequence that avoid requests overlapping more than necessary:

  • Use the last time seen (easier if the last page has newer data than the first page, but possible either way)
  • Reuse the last nextLink from the previous sequence (only possible if pagination goes forward in time)
  • Set the endDateTime to be e.g. now - 1m, then use the last endDateTime as the next startDateTime. This one is similar to lookback, except instead of looking at the unreliable time range twice, we avoid looking at it at all.

Since we know we're missing data, I think the new default settings should avoid that. Setting a 0s default lookback allows for manual experimentation, but I'd expect the problem to come up again.

@chrisberkhout chrisberkhout left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Happy to discuss further if helpful

@ShourieG

ShourieG commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

Hi, I also reported this problem and found that:

  1. The events returned by the API are sorted in decreasing order of time and can include events with eventTimeDT equal to the two search extremes (startDate and endDate)
  2. subsequent readings should restart with startDate: eventTimeDT of the last event stored in the previous reading session (5 minutes before, since interval: 5m in configuration). This can result in the same event being read twice.
  3. It would be necessary to provide for the management of a bookmark in the readings in the sessions, taking care to verify that the events read with eventTimeDT=startDate have not already been stored (for example, you can compare the uuid value of the bookmark event with that of the events read subsequently)

Hi @cmarlettalivi, just wanted some more clarification on your 1st statement. You said that "The events returned by the API are sorted in decreasing order of time". That would mean that the latest event would be the 1st one in every request right ? Cause generally events are sorted in ascending order, so just confirming that this is indeed the case.

@cmarlettalivi

Copy link
Copy Markdown

@ShourieG
the events are sorted in descending order, that is, the first one that appears in the response is the most recent.

@cmarlettalivi

cmarlettalivi commented Jan 21, 2025

Copy link
Copy Markdown

Hi @chrisberkhout ,
have you considered using, for each reading session, the timestamp value (eventTimeDT) of the last stored event as startDate? This would imply storing that timestamp as a bookmark somewhere. Since it is clear that the events are sorted in descending order in the response, we would not have any event loss. I think this is more accurate than setting endDateTime to now - 1m and using the last endDateTime as the next startDateTime.

@ShourieG

ShourieG commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

@chrisberkhout, @cmarlettalivi, I'm closing this PR, new PR with the relevant changes are up: #12425

@ShourieG ShourieG closed this Jan 22, 2025
@ShourieG
ShourieG deleted the enhancement/vision_one branch January 22, 2025 11:00
@ShourieG
ShourieG restored the enhancement/vision_one branch February 3, 2025 12:21
@ShourieG ShourieG reopened this Feb 3, 2025
@ShourieG

ShourieG commented Feb 3, 2025

Copy link
Copy Markdown
Contributor Author

NOTE: The timestamp update did not completely solve the missing documents issue, it did reduce but still some are missing and trend micro confirmed that events are populated with a delay sometimes, to address this I'm reopening this PR and giving the customer the option to tweak a custom look-back period.

@elasticmachine

Copy link
Copy Markdown

💚 Build Succeeded

History

  • 💔 Build #21457 failed 9009a08e768494ea3faf38f6e1db08795c6730d7
  • 💚 Build #20606 succeeded 90ff2d426f9f8c4b4fc789200d82b037ce957753

cc @ShourieG

@ShourieG
ShourieG merged this pull request into elastic:main Feb 3, 2025
@ShourieG
ShourieG deleted the enhancement/vision_one branch February 3, 2025 16:21
@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

Package trend_micro_vision_one - 1.25.0 containing this change is available at https://epr.elastic.co/package/trend_micro_vision_one/1.25.0/

qcorporation pushed a commit that referenced this pull request Feb 3, 2025
harnish-crest-data pushed a commit to chavdaharnish/integrations that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Integration:trend_micro_vision_one TrendAI Vision One integration Label used for meta issues tracking each integration Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]

4 participants