-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[Sampling] add get call for sampling configs #136912
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
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
This PR adds a GET endpoint to retrieve sampling configuration for a specific index. The implementation follows the existing patterns established by the PUT and DELETE operations for sampling configurations.
Key Changes
- Introduces
GetSampleConfigurationActionwith request/response classes for retrieving sampling config - Implements REST and transport layer handlers for the new GET endpoint at
/{index}/_sample/config - Adds comprehensive test coverage including unit tests, integration tests, and REST API spec tests
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Constants.java | Adds the new action permission string for the GET operation |
| GetSampleConfigurationActionResponseTests.java | Unit tests for response serialization and mutation |
| GetSampleConfigurationActionRequestTests.java | Unit tests for request validation and behavior |
| TransportGetSampleConfigurationAction.java | Core transport action that retrieves config from cluster metadata |
| RestGetSampleConfigurationAction.java | REST handler for the GET endpoint |
| GetSampleConfigurationAction.java | Action definition with Request/Response classes |
| ActionModule.java | Registers the new action and REST handler |
| GetSampleConfigurationActionIT.java | Integration tests for various GET scenarios |
| 10_basic.yml | REST API specification tests |
| indices.get_sample_configuration.json | REST API endpoint specification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...n/java/org/elasticsearch/action/admin/indices/sampling/RestGetSampleConfigurationAction.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/action/admin/indices/sampling/RestGetSampleConfigurationAction.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/action/admin/indices/sampling/GetSampleConfigurationAction.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-data-management (Team:Data Management) |
| /** | ||
| * The name identifier for this action type used in the transport layer. | ||
| */ | ||
| public static final String NAME = "indices:admin/sample/config/get"; |
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 think we want this to be "indices:monitor/sample/config/get"
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.
gotcha, does that influence anything behind the scenes or is it just a naming convention thing?
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.
The permissions are based off of it. We want fetching the config to only require the index monitor permission (vs index admin to create or modify or delete it).
| "indices:admin/sample/stats", | ||
| "indices:admin/sample/config/delete" | ||
| "indices:admin/sample/config/delete", | ||
| "indices:admin/sample/config/get" |
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.
Same as above, I think this needs to be monitor.
masseyke
left a comment
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.
LGTM
* add get call for configs * wire in handlers * update comments * update action name --------- Co-authored-by: Keith Massey <keith.massey@elastic.co>
BASE=fd70808de0425fc40064be374748c8648e297926 HEAD=68aeacfc242c3e6a59b6424612e5acd9a249b3a1 Branch=main
BASE=fd70808de0425fc40064be374748c8648e297926 HEAD=68aeacfc242c3e6a59b6424612e5acd9a249b3a1 Branch=main
Add GET endpoint for sampling config:
e.g.
Get Configuration
The GET /index-name/_sample/config API will get a specific configuration. For example:
GET /logs/_sample/config,
returns