Support creating/updating prometheus rules

[lezzago]

Description

Adds CRUD support for the Prometheus/Cortex/AMP Ruler API and Alertmanager enhancements through OpenSearch's direct query resources framework.

Prometheus Rules — enables management of recording and alerting rule groups via the Cortex/Thanos Ruler API, compatible with Amazon Managed Prometheus (AMP), Cortex, Thanos, and Grafana Mimir.

Alertmanager — adds support for fetching Alertmanager status/configuration and expiring silences.

Vanilla Prometheus does not support the Ruler API (it manages rules via config files). Write/delete operations return clear errors when used against vanilla Prometheus. The existing GET /api/v1/rules (all rules) continues to work against all backends.

New API Endpoints

Rules — under /_plugins/_directquery/_resources/{dataSource}/api/v1/rules/:

Method	Path	Operation
GET	/{namespace}	Retrieve rules for a specific namespace
POST	/{namespace}	Create or update a rule group (YAML body)
DELETE	/{namespace}	Delete all rules in a namespace
DELETE	/{namespace}/{groupName}	Delete a specific rule group

Alertmanager — under /_plugins/_directquery/_resources/{dataSource}/alertmanager/api/v2/:

Method	Path	Operation
GET	/status	Get Alertmanager status, configuration, and cluster info
DELETE	/silence/{silenceID}	Expire (delete) a specific silence

Response Normalization

Different backends return rules in different formats — Prometheus/AMP return JSON, Cortex/Thanos return YAML. A normalizeRulesResponse() method converts all formats into a consistent {"groups":[...]} JSON structure with a file field on each group indicating the namespace. The existing getRules() return type remains JSONObject (no breaking change).

Configurable Ruler URI

A new optional prometheus.ruler.uri datasource property provides a separate base URL for Ruler CRUD operations. This is needed because Cortex serves the Ruler API at a different base path than the Prometheus-compatible query API. When not set, defaults to prometheus.uri (works for AMP out of the box). Fully backward compatible — existing datasources work without changes.

Example Cortex configuration:

{
  "prometheus.uri": "http://cortex:9009/api/prom",
  "prometheus.ruler.uri": "http://cortex:9009"
}

Design Decisions

• Unified write path: DELETE operations flow through the existing writeDirectQueryResources transport action using a delete flag on WriteDirectQueryResourcesRequest, avoiding a separate delete transport layer.
• YAML passthrough: Rule creation accepts YAML bodies passed through to the Ruler API with Content-Type: application/yaml.
• Rolling-upgrade safe: WriteDirectQueryResourcesRequest extended with groupName and delete fields using readOptionalString/writeOptionalBoolean for serialization safety.

Other Changes

• Extracted readRulerResponse helper to consolidate duplicated HTTP error handling across ruler methods
• Added try-with-resources on all OkHttp Response objects in ruler methods to prevent connection leaks
• Input validation on namespace and groupName at the REST converter level
• Route detection checks both path and parameter presence to avoid misrouting
• ALERTMANAGER_STATUS added to DirectQueryResourceType enum

Related Issues

N/A

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b0ab3b5)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections direct-query/src/main/java/org/opensearch/sql/directquery/rest/RestDirectQueryResourcesManagementAction.java [65]: Update routes to be more generic and move away from prometheus direct-query/src/main/java/org/opensearch/sql/directquery/transport/format/DirectQueryResourcesRequestConverter.java [32]: Move prometheus code into prometheus module/classes

🔀 Multiple PR themes Sub-PR theme: Add Alertmanager silence delete and status endpoints to PrometheusClient Relevant files: direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClient.java direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java direct-query-core/src/test/java/org/opensearch/sql/prometheus/client/PrometheusClientImplTest.java Sub-PR theme: Add Prometheus Ruler API CRUD support for rule groups and namespaces Relevant files: direct-query-core/src/main/java/org/opensearch/sql/directquery/rest/model/WriteDirectQueryResourcesRequest.java direct-query-core/src/main/java/org/opensearch/sql/directquery/rest/model/DirectQueryResourceType.java direct-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.java direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java direct-query-core/src/main/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandler.java direct-query/src/main/java/org/opensearch/sql/directquery/rest/RestDirectQueryResourcesManagementAction.java direct-query/src/main/java/org/opensearch/sql/directquery/transport/format/DirectQueryResourcesRequestConverter.java direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/WriteDirectQueryResourcesActionRequest.java direct-query-core/src/test/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandlerTest.java direct-query-core/src/test/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtilsTest.java direct-query/src/test/java/org/opensearch/sql/directquery/rest/RestDirectQueryResourcesManagementActionTest.java direct-query/src/test/java/org/opensearch/sql/directquery/transport/format/DirectQueryResourcesRequestConverterTest.java direct-query/src/test/java/org/opensearch/sql/directquery/transport/model/WriteDirectQueryResourcesActionRequestTest.java

⚡ Recommended focus areas for review Resource Leak In deleteAlertmanagerSilence and getAlertmanagerStatus, the Response object is obtained via AccessController.doPrivilegedChecked but not wrapped in a try-with-resources block. If an exception occurs after the response is obtained but before it is fully consumed, the response body may not be closed, causing a resource leak. The newer Ruler methods correctly use try-with-resources, but these two methods do not. Response response = AccessController.doPrivilegedChecked( () -> this.alertmanagerHttpClient.newCall(request).execute()); if (response.isSuccessful()) { return "{\"status\":\"success\"}"; } else { String errorBody = response.body() != null ? response.body().string() : "No response body"; logger.error( "Delete Alertmanager Silence request failed with code: {}, error body: {}", response.code(), errorBody); throw new PrometheusClientException( String.format( "Alertmanager request failed with code: %s. Error details: %s", response.code(), errorBody)); } } @Override public JSONObject getAlertmanagerStatus() throws IOException { String baseUrl = alertmanagerUri.toString().replaceAll("/$", ""); String queryUrl = String.format("%s/api/v2/status", baseUrl); logger.debug("Making Alertmanager status request: {}", queryUrl); Request request = new Request.Builder().url(queryUrl).build(); Response response = AccessController.doPrivilegedChecked( () -> this.alertmanagerHttpClient.newCall(request).execute()); if (response.isSuccessful()) { String bodyString = Objects.requireNonNull(response.body()).string(); logger.debug("Alertmanager status response body: {}", bodyString); return new JSONObject(bodyString); } else { String errorBody = response.body() != null ? response.body().string() : "No response body"; logger.error( "Alertmanager status request failed with code: {}, error body: {}", response.code(), errorBody); throw new PrometheusClientException( String.format( "Alertmanager request failed with code: %s. Error details: %s", response.code(), errorBody)); } } Null Body Handling In readRulerResponse, when the response is successful, Objects.requireNonNull is used which throws NullPointerException (not PrometheusClientException) if the body is null. This is inconsistent with the error path which returns "No response body". A null body on a successful response would surface as an unexpected NPE rather than a handled exception. if (response.isSuccessful()) { return Objects.requireNonNull(response.body(), "Ruler response body is null").string(); Wrong HTTP Client In getRulesByNamespace, createOrUpdateRuleGroup, deleteRuleNamespace, and deleteRuleGroup, the prometheusHttpClient is used to make requests to the rulerUri. If the ruler URI points to a different host (e.g., Cortex at a different base path), the authentication/client configuration for that host may differ. There should be a dedicated rulerHttpClient or at least documentation clarifying this limitation. try (Response response = AccessController.doPrivilegedChecked( () -> this.prometheusHttpClient.newCall(request).execute())) { String body = readRulerResponse(response, "GET namespace " + namespace); return normalizeRulesResponse(body, namespace); } } @Override public String createOrUpdateRuleGroup(String namespace, String yamlBody) throws IOException { String queryUrl = String.format( "%s/api/v1/rules/%s", rulerUri.toString().replaceAll("/$", ""), URLEncoder.encode(namespace, StandardCharsets.UTF_8)); logger.debug("Making Ruler POST request to create/update rule group"); Request request = new Request.Builder() .url(queryUrl) .header("Content-Type", "application/yaml") .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8))) .build(); try (Response response = AccessController.doPrivilegedChecked( () -> this.prometheusHttpClient.newCall(request).execute())) { String body = readRulerResponse(response, "POST create/update rule group"); return body.isEmpty() ? "{\"status\":\"success\"}" : body; } } @Override public String deleteRuleNamespace(String namespace) throws IOException { String queryUrl = String.format( "%s/api/v1/rules/%s", rulerUri.toString().replaceAll("/$", ""), URLEncoder.encode(namespace, StandardCharsets.UTF_8)); logger.debug("Making Ruler DELETE request for namespace"); Request request = new Request.Builder().url(queryUrl).delete().build(); try (Response response = AccessController.doPrivilegedChecked( () -> this.prometheusHttpClient.newCall(request).execute())) { String body = readRulerResponse(response, "DELETE namespace " + namespace); return body.isEmpty() ? "{\"status\":\"success\"}" : body; } } @Override public String deleteRuleGroup(String namespace, String groupName) throws IOException { String queryUrl = String.format( "%s/api/v1/rules/%s/%s", rulerUri.toString().replaceAll("/$", ""), URLEncoder.encode(namespace, StandardCharsets.UTF_8), URLEncoder.encode(groupName, StandardCharsets.UTF_8)); logger.debug("Making Ruler DELETE request for group"); Request request = new Request.Builder().url(queryUrl).delete().build(); try (Response response = AccessController.doPrivilegedChecked( () -> this.prometheusHttpClient.newCall(request).execute())) { String body = readRulerResponse(response, "DELETE group " + groupName); return body.isEmpty() ? "{\"status\":\"success\"}" : body; } } Missing Route Check In toWriteDirectRestRequest, the alertmanager path handling checks for /silence/ and /alerts/groups but the original code only handled /alerts/groups as a special case. The new code sets ALERTMANAGER_ALERT_GROUPS for POST to /alerts/groups, which may not be a valid write operation. Additionally, if neither condition matches, it falls through to the fromString conversion which could produce unexpected resource types. if (path.contains("/silence/")) { // DELETE /alertmanager/api/v2/silence/{silenceID} directQueryRequest.setResourceType(DirectQueryResourceType.ALERTMANAGER_SILENCES); directQueryRequest.setResourceName(restRequest.param("silenceID")); } else if (path.contains("/alerts/groups")) { directQueryRequest.setResourceType(DirectQueryResourceType.ALERTMANAGER_ALERT_GROUPS); } else { directQueryRequest.setResourceType( DirectQueryResourceType.fromString( "alertmanager_" + restRequest.param("resourceType"))); } Naming Convention The field RequestOptions (line 23) uses PascalCase instead of camelCase (requestOptions), which violates Java naming conventions. This appears to be a pre-existing issue not introduced in this PR, but the new fields groupName and delete follow correct conventions. The inconsistency may cause issues with serialization frameworks. private Map<String, String> RequestOptions; private String groupName; private boolean delete;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b0ab3b5

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix response resource leak in HTTP methods The deleteAlertmanagerSilence and getAlertmanagerStatus methods do not use try-with-resources for the Response object, which can cause resource leaks. Unlike the Ruler API methods which correctly use try (Response response = ...), these methods hold the response without closing it. Wrap the response in a try-with-resources block. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [348-355] -Response response = +try (Response response = AccessController.doPrivilegedChecked( - () -> this.alertmanagerHttpClient.newCall(request).execute()); + () -> this.alertmanagerHttpClient.newCall(request).execute())) { + if (response.isSuccessful()) { + return "{\"status\":\"success\"}"; + } else { + String errorBody = response.body() != null ? response.body().string() : "No response body"; -if (response.isSuccessful()) { - return "{\"status\":\"success\"}"; -} else { - String errorBody = response.body() != null ? response.body().string() : "No response body"; - Suggestion importance[1-10]: 8 __ Why: The deleteAlertmanagerSilence and getAlertmanagerStatus methods don't use try-with-resources for the Response object, unlike the Ruler API methods which correctly do. This is a genuine resource leak that could cause connection pool exhaustion in production.	Medium
	Guard against ClassCastException in YAML list parsing The unchecked cast (List<Map<String, Object>>) parsed can throw a ClassCastException at runtime if the list contains non-Map elements (e.g., plain strings). The test testGetRulesYamlParseExceptionReturnsEmptyGroups confirms this scenario exists. The cast should be guarded with an instanceof check per element to avoid propagating the exception up to the caller. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [556-566] } else if (parsed instanceof List) { // Single-namespace format: List<RuleGroup> - List<Map<String, Object>> groups = (List<Map<String, Object>>) parsed; - for (Map<String, Object> group : groups) { + List<?> rawList = (List<?>) parsed; + for (Object item : rawList) { + if (!(item instanceof Map)) { + throw new ClassCastException("Expected Map but got: " + item.getClass().getName()); + } + Map<String, Object> group = (Map<String, Object>) item; JSONObject groupObj = new JSONObject(group); if (namespace != null) { groupObj.put("file", namespace); } groupsArray.put(groupObj); } } Suggestion importance[1-10]: 6 __ Why: The unchecked cast to List<Map<String, Object>> can throw a ClassCastException if the list contains non-Map elements. The test testGetRulesYamlParseExceptionReturnsEmptyGroups confirms this scenario, and the outer catch (Exception e) in normalizeRulesResponse already handles it gracefully, but the suggestion makes the code more explicit and safer. However, the improved code re-throws the exception rather than letting the outer catch handle it, which changes behavior.	Low
	Handle null body consistently on successful response In readRulerResponse, when the response is successful, Objects.requireNonNull is used which throws NullPointerException instead of a meaningful error. This is inconsistent with the error path which gracefully handles null bodies. Consider returning an empty string or a consistent fallback when the body is null on success, similar to how the error path handles it. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [483-486] private String readRulerResponse(Response response, String operationDescription) throws IOException { if (response.isSuccessful()) { - return Objects.requireNonNull(response.body(), "Ruler response body is null").string(); + return response.body() != null ? response.body().string() : ""; } else { String errorBody = response.body() != null ? response.body().string() : "No response body"; Suggestion importance[1-10]: 5 __ Why: The readRulerResponse method uses Objects.requireNonNull on success which throws NullPointerException instead of a meaningful error, while the error path gracefully handles null bodies. The suggestion improves consistency, though the test testGetRulesHttpErrorWithNullBody already tests the null body error path.	Low
General	Improve null body handling with descriptive error In getAlertmanagerStatus, Objects.requireNonNull(response.body()) will throw a NullPointerException with no meaningful message if the body is null on a successful response. This is inconsistent with other methods and will produce an unhelpful error. Add a null check with a descriptive fallback or error message. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [378-381] if (response.isSuccessful()) { - String bodyString = Objects.requireNonNull(response.body()).string(); + if (response.body() == null) { + throw new PrometheusClientException("Alertmanager status response body is null"); + } + String bodyString = response.body().string(); logger.debug("Alertmanager status response body: {}", bodyString); return new JSONObject(bodyString); Suggestion importance[1-10]: 5 __ Why: Objects.requireNonNull(response.body()) in getAlertmanagerStatus throws a generic NullPointerException with no meaningful message, which is inconsistent with other methods. The suggestion improves error clarity, though this is a minor quality issue.	Low

---

Previous suggestions

Suggestions up to commit b5480e9

Category	Suggestion	Impact
Possible issue	Handle null body on successful response gracefully When response.isSuccessful() is true but response.body() is null, Objects.requireNonNull throws a NullPointerException instead of a PrometheusClientException, breaking the consistent error-handling contract. Consider returning an empty string (or throwing a PrometheusClientException) when the body is null on a successful response, similar to how the error path handles it. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [426-429] if (response.isSuccessful()) { - return Objects.requireNonNull(response.body(), "Ruler response body is null").string(); + return response.body() != null ? response.body().string() : ""; } else { String errorBody = response.body() != null ? response.body().string() : "No response body"; Suggestion importance[1-10]: 6 __ Why: When response.isSuccessful() is true but response.body() is null, Objects.requireNonNull throws a NullPointerException rather than a PrometheusClientException, breaking the consistent error-handling contract. The fix is valid and improves robustness, though this is an edge case.	Low
General	Set MediaType directly on RequestBody for YAML POST RequestBody.create(byte[]) is deprecated in newer OkHttp versions and does not set a MediaType, which may cause the server to reject the request or misinterpret the content type even though the Content-Type header is set separately. Pass the MediaType directly to RequestBody.create to ensure it is correctly associated with the body. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [366-371] Request request = new Request.Builder() .url(queryUrl) - .header("Content-Type", "application/yaml") - .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8))) + .post(RequestBody.create( + yamlBody.getBytes(StandardCharsets.UTF_8), + okhttp3.MediaType.parse("application/yaml"))) .build(); Suggestion importance[1-10]: 5 __ Why: Using RequestBody.create(byte[]) without a MediaType is deprecated and may cause issues with some servers. Passing the MediaType directly to RequestBody.create is the correct approach, though the separate Content-Type header partially mitigates the issue.	Low
	Guard against non-Map elements in YAML list parsing The unchecked cast (List<Map<String, Object>>) parsed will succeed even if the list contains non-Map elements (e.g., plain strings), causing a ClassCastException inside the loop when new JSONObject(group) is called. The outer catch (Exception e) in normalizeRulesResponse will swallow this silently, but it is better to guard against it explicitly with an instanceof check per element. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [499-503] } else if (parsed instanceof List) { // Single-namespace format: List<RuleGroup> - List<Map<String, Object>> groups = (List<Map<String, Object>>) parsed; - for (Map<String, Object> group : groups) { - JSONObject groupObj = new JSONObject(group); + List<?> rawGroups = (List<?>) parsed; + for (Object item : rawGroups) { + if (!(item instanceof Map)) { + throw new IllegalArgumentException("Unexpected list element type: " + item.getClass()); + } + JSONObject groupObj = new JSONObject((Map<?, ?>) item); Suggestion importance[1-10]: 4 __ Why: The unchecked cast can cause a ClassCastException if the list contains non-Map elements, which is silently swallowed by the outer catch. The suggestion improves robustness, though the outer catch already handles this gracefully by returning empty groups.	Low
	Simplify nullable Boolean unboxing to avoid potential NPE The new fields groupName and delete are read after requestOptions in the deserialization constructor, but they must also be written in the exact same order in writeTo. Verify that writeTo writes groupName and delete after requestOptions; if the order ever diverges between the two methods, deserialization will silently corrupt all fields that follow. Additionally, deleteValue != null ? deleteValue : false can be simplified to Boolean.TRUE.equals(deleteValue). direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/WriteDirectQueryResourcesActionRequest.java [41-43] request.setGroupName(in.readOptionalString()); -Boolean deleteValue = in.readOptionalBoolean(); -request.setDelete(deleteValue != null ? deleteValue : false); +request.setDelete(Boolean.TRUE.equals(in.readOptionalBoolean())); Suggestion importance[1-10]: 3 __ Why: The suggestion to use Boolean.TRUE.equals(in.readOptionalBoolean()) is a minor simplification of the existing null-safe unboxing pattern. The existing code is already correct and safe, so this is a minor style improvement.	Low

Suggestions up to commit c047583

Category	Suggestion	Impact
Possible issue	Avoid NPE on null successful response body Objects.requireNonNull(response.body()) will throw a NullPointerException if the response body is null on a successful response, rather than a meaningful error. The error-path already handles null body gracefully with a null check, so the success path should be consistent. Use a null-safe check here as well. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [243-244] if (response.isSuccessful()) { - String bodyString = Objects.requireNonNull(response.body()).string(); + if (response.body() == null) { + throw new PrometheusClientException("Ruler request returned a successful response but with no body"); + } + String bodyString = response.body().string(); Suggestion importance[1-10]: 6 __ Why: Using Objects.requireNonNull on a successful response body throws a generic NullPointerException rather than a meaningful PrometheusClientException. The error path already handles null body gracefully, so the success path should be consistent. This is a valid improvement but not a critical bug since successful responses typically have a body.	Low
General	Handle backward compatibility in stream deserialization The read order in the constructor must exactly match the write order in writeTo. Currently writeTo writes groupName then delete, and the constructor reads them in the same order — this is correct. However, readBoolean will throw if the stream ends unexpectedly (e.g., when deserializing older serialized data that doesn't have these fields). Consider using in.readOptionalBoolean() or adding a version check for backward compatibility. direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/WriteDirectQueryResourcesActionRequest.java [41-42] request.setGroupName(in.readOptionalString()); -request.setDelete(in.readBoolean()); +Boolean deleteValue = in.readOptionalBoolean(); +request.setDelete(deleteValue != null && deleteValue); Suggestion importance[1-10]: 5 __ Why: Using readBoolean() instead of readOptionalBoolean() could cause deserialization failures when reading older serialized data that doesn't include the new delete field. This is a valid backward compatibility concern for distributed systems where nodes may be running different versions.	Low
	Associate MediaType directly with request body RequestBody.create(byte[]) is deprecated in newer OkHttp versions and does not associate a MediaType with the body. The Content-Type header is set separately, but it's better practice to pass the MediaType directly to RequestBody.create to ensure consistency and avoid potential issues with some HTTP clients or proxies. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [380-385] +MediaType yamlMediaType = MediaType.parse("application/yaml"); Request request = new Request.Builder() .url(queryUrl) - .header("Content-Type", "application/yaml") - .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8))) + .post(RequestBody.create(yamlBody, yamlMediaType)) .build(); Suggestion importance[1-10]: 4 __ Why: Using RequestBody.create(byte[]) without a MediaType is deprecated in newer OkHttp versions and sets the Content-Type header separately. While functional, passing MediaType directly to RequestBody.create is better practice and more consistent, but this is a minor style/API usage improvement.	Low
	Fix field naming convention violation The field RequestOptions violates Java naming conventions — field names should start with a lowercase letter. While this is pre-existing, the new fields groupName and delete are correctly named. If RequestOptions is serialized/deserialized (e.g., via Jackson), the inconsistent naming may cause issues when the new fields are also serialized. Ensure the class-level serialization annotations are consistent for all fields. direct-query-core/src/main/java/org/opensearch/sql/directquery/rest/model/WriteDirectQueryResourcesRequest.java [23-25] -private Map<String, String> RequestOptions; +private Map<String, String> requestOptions; private String groupName; private boolean delete; Suggestion importance[1-10]: 2 __ Why: The RequestOptions field naming violation is pre-existing and not introduced by this PR. The suggestion to rename it could break existing serialization/deserialization behavior and is out of scope for this PR's changes.	Low

Suggestions up to commit 3a17d23

Category	Suggestion	Impact
Possible issue	Add null check before accessing JSON field The getRulesByNamespace method calls readResponse which may throw an exception if the response is not successful, but if the response body doesn't contain a "data" key, a JSONException will be thrown without a meaningful error message. Add a null/existence check before accessing the "data" key to provide a clearer error. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [335-336] JSONObject jsonObject = readResponse(response); +if (!jsonObject.has("data")) { + throw new PrometheusClientException("Unexpected response format: missing 'data' field"); +} return jsonObject.getJSONObject("data"); Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - accessing jsonObject.getJSONObject("data") without checking if the key exists could throw an unhelpful JSONException. However, this is a minor defensive coding improvement rather than a critical bug fix.	Low
	Ensure serialization order matches deserialization order The serialization order in writeTo must exactly match the deserialization order in the constructor. The new fields groupName and delete are written after requestOptions in writeTo, but they must also be read in the same order. Verify that the writeTo method writes groupName before delete to match this read order, otherwise deserialization will be corrupted. direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/WriteDirectQueryResourcesActionRequest.java [41-42] +request.setGroupName(in.readOptionalString()); +request.setDelete(in.readBoolean()); - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical, making this a verification suggestion rather than an actual code change. The PR already shows writeTo writes groupName before delete (lines 62-63), matching the read order, so the concern is already addressed.	Low
General	Use MediaType in RequestBody creation The RequestBody.create(byte[]) method without a MediaType parameter is deprecated in newer OkHttp versions and may not set the content type correctly. The Content-Type header is set separately, but it's better to pass the MediaType directly to RequestBody.create to ensure consistency. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [347-352] +MediaType yamlMediaType = MediaType.parse("application/yaml"); Request request = new Request.Builder() .url(queryUrl) - .header("Content-Type", "application/yaml") - .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8))) + .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8), yamlMediaType)) .build(); Suggestion importance[1-10]: 4 __ Why: The suggestion to pass MediaType directly to RequestBody.create is a valid improvement for consistency and to avoid potential deprecation issues, but it's a minor style/API usage improvement rather than a critical fix.	Low
	Fix field naming convention inconsistency The field RequestOptions violates Java naming conventions (should be requestOptions with a lowercase first letter). While this may be pre-existing, the new fields groupName and delete follow correct conventions. This inconsistency could cause issues with JSON deserialization frameworks that rely on field naming conventions. direct-query-core/src/main/java/org/opensearch/sql/directquery/rest/model/WriteDirectQueryResourcesRequest.java [23-25] -private Map<String, String> RequestOptions; +private Map<String, String> requestOptions; private String groupName; private boolean delete; Suggestion importance[1-10]: 3 __ Why: The RequestOptions field naming violation is pre-existing and not introduced by this PR. The suggestion is valid for code quality but is out of scope for this PR and could break existing code that references this field.	Low

Suggestions up to commit 98d6a33

Category	Suggestion	Impact
Possible issue	Guard against missing 'data' key in response The readResponse method may throw an exception if the response is not successful, but if it returns a JSONObject that doesn't contain a "data" key, calling getJSONObject("data") will throw a JSONException. Add a null/key check before accessing the "data" field to avoid unexpected runtime exceptions. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [335-337] Response response = AccessController.doPrivilegedChecked( () -> this.prometheusHttpClient.newCall(request).execute()); JSONObject jsonObject = readResponse(response); +if (!jsonObject.has("data")) { + throw new PrometheusClientException("Unexpected response format: missing 'data' field"); +} return jsonObject.getJSONObject("data"); Suggestion importance[1-10]: 6 __ Why: If the response JSON doesn't contain a "data" key, getJSONObject("data") will throw a JSONException at runtime. Adding a key check before accessing it would make the error handling more explicit and informative.	Low
General	Set media type on request body correctly RequestBody.create(byte[]) is deprecated in newer OkHttp versions and the overload without a MediaType may not set the content type correctly on the wire. The Content-Type header is set separately, but the RequestBody itself should also carry the media type to ensure correct behavior. Pass the media type directly to RequestBody.create. direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java [347-352] Request request = new Request.Builder() .url(queryUrl) - .header("Content-Type", "application/yaml") - .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8))) + .post(RequestBody.create(yamlBody.getBytes(StandardCharsets.UTF_8), + okhttp3.MediaType.parse("application/yaml"))) .build(); Suggestion importance[1-10]: 4 __ Why: The Content-Type header is set separately via .header(), but the RequestBody itself doesn't carry the media type. While this may work in practice, passing the MediaType directly to RequestBody.create is the more correct and idiomatic OkHttp approach.	Low
	Derive resource type from request path dynamically The toDeleteDirectRestRequest method hardcodes DirectQueryResourceType.RULES regardless of the request path or parameters. If additional resource types are supported for DELETE in the future, this will silently misclassify requests. Consider deriving the resource type from the request path or a parameter, consistent with how toGetDirectRestRequest and toWriteDirectRestRequest handle routing. direct-query/src/main/java/org/opensearch/sql/directquery/transport/format/DirectQueryResourcesRequestConverter.java [117-122] -deleteRequest.setResourceType(DirectQueryResourceType.RULES); +String path = restRequest.path(); +if (path.contains("/api/v1/rules/")) { + deleteRequest.setResourceType(DirectQueryResourceType.RULES); +} else { + deleteRequest.setResourceTypeFromString(restRequest.param("resourceType")); +} String namespace = restRequest.param("namespace"); if (namespace == null || namespace.isEmpty()) { throw new IllegalArgumentException("Namespace is required for delete rule operations"); } Suggestion importance[1-10]: 3 __ Why: The hardcoded RULES resource type is consistent with the current DELETE routes which only support rules, so this is a minor extensibility concern rather than a current bug. The suggestion is valid for future-proofing but has low immediate impact.	Low

[github-actions]

Persistent review updated to latest commit 3a17d23

[github-actions]

Persistent review updated to latest commit c047583

[github-actions]

Persistent review updated to latest commit b5480e9

[github-actions]

Persistent review updated to latest commit b0ab3b5