-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add creationDate to SamplingConfiguration #136442
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
seanzatzdev
commented
Oct 11, 2025
- Adds creationDate to SamplingConfiguration
- User generated creation dates are rejected.
- Adds a human readable field for the creation date to the XContent representation
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.
Pull Request Overview
Adds timestamp tracking functionality to SamplingConfiguration by introducing a creationDate field that automatically records when configurations are created, while preventing users from manually setting creation dates.
- Adds a creationDate field to the SamplingConfiguration record with automatic timestamp generation
- Implements validation to reject user-provided creation dates via context-aware parsing
- Updates serialization methods to include the new creationDate field
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
SamplingConfiguration.java | Adds creationDate field, updates constructors, parsing logic, and serialization methods |
RestPutSampleConfigurationAction.java | Updates to use user-data aware parsing method to prevent manual creation date setting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
…mpling/SamplingConfiguration.java Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-data-management (Team:Data Management) |
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfigurationTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfigurationTests.java
Outdated
Show resolved
Hide resolved
ObjectParser.ValueType.LONG | ||
); | ||
PARSER.declareString(optionalConstructorArg(), new ParseField(CONDITION_FIELD_NAME)); | ||
PARSER.declareField(optionalConstructorArg(), (p, c) -> { |
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.
You can just delete this (and adjust the array index of rawCreationTime above), right? It doesn't seem to actually be used.
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.
Oh I see it does make for a more meaningful user error message when they try to set creation_time.
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.
Yeah, but also since we're using builder.timestampFieldsFromUnixEpochMillis(), it can lead to some weird errors if you don't add parsing instructions for both the raw and human readable fields. so i just added parsing instructions + ignored the human readable one up above
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfiguration.java
Outdated
Show resolved
Hide resolved
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.
I left a couple of minor comments, but LGTM
# Conflicts: # server/src/test/java/org/elasticsearch/action/admin/indices/sampling/SamplingConfigurationTests.java