Skip to content

Conversation

@przemekwitek
Copy link
Contributor

@przemekwitek przemekwitek commented Jun 5, 2025

This PR fixes NPE thrown by the DatafeedDelayedDataDetector.checkCurrentBucketEventCount method when the date_buckets aggregation is missing in the response.

@przemekwitek przemekwitek added :ml Machine learning Team:ML Meta label for the ML team v8.18.0 v9.1.0 labels Jun 5, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

List<? extends Histogram.Bucket> buckets = ((Histogram) response.getAggregations().get(DATE_BUCKETS)).getBuckets();
Histogram histogram = searchResponse.getAggregations().get(DATE_BUCKETS);
if (histogram == null) {
logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

We expect this to be a rare occurrence so let's log it at a higher level. Also we want to know if the delayed detector is not working

Suggested change
logger.debug(
logger.warn(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just double-checking: If we log at WARN, we shouldn't include full request/response bodies, should we?

Copy link
Member

Choose a reason for hiding this comment

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

hmm probably not, we know what the request is anyway as it's fixed in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second thought, I added logging of "search response" because if the agg is missing, the response is never big and we can get more useful info this way.

@elasticsearchmachine
Copy link
Collaborator

Hi @przemekwitek, I've created a changelog YAML for you.

@przemekwitek
Copy link
Contributor Author

run elasticsearch-ci/part-2

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM thanks for making the changes

@przemekwitek przemekwitek added the auto-backport Automatically create backport pull requests when merged label Jun 6, 2025
@przemekwitek przemekwitek merged commit 9cfd293 into elastic:main Jun 6, 2025
18 checks passed
@przemekwitek przemekwitek deleted the delayed_data_fix branch June 6, 2025 13:46
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 128974

@przemekwitek
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.18

Questions ?

Please refer to the Backport tool documentation

@przemekwitek
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.19

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged backport pending >bug :ml Machine learning Team:ML Meta label for the ML team v8.18.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants