Add explicit index template mappings and reporting config wiring for OpenSearchMetricsSink#2569
Add explicit index template mappings and reporting config wiring for OpenSearchMetricsSink#2569Shivani-techno wants to merge 12 commits intoopensearch-project:mainfrom
Conversation
|
Please fix the DCO failure. |
| shim-solr-primary: | ||
| <<: *shim-base | ||
| volumes: | ||
| - transform-dist:/transforms:ro |
There was a problem hiding this comment.
Why was this change required?
There was a problem hiding this comment.
The volume override is scoped to shim-solr-primary ( dual mode ) only, not the base. The transform-dist volume is re-declared because YAML merge replaces rather than appends — without it, the transform JS files wouldn't be mounted.
| <<: *shim-base | ||
| volumes: | ||
| - transform-dist:/transforms:ro | ||
| - ./reporting-config.yaml:/config/reporting-config.yaml:ro |
There was a problem hiding this comment.
Reporting config makes sense only when we are running in dual mode. Adding it here will mount the config on all the shims - it will largely remain unused.
| } | ||
|
|
||
| /** Simple YAML flattener — handles indentation-based nesting, strips comments. */ | ||
| private static Map<String, String> flattenYaml(String content) { |
There was a problem hiding this comment.
We should really consider using a YAML parsing library. YAML is not just indent based line parsing.
There was a problem hiding this comment.
Agreed. Raising the revision to use parsing library.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2569 +/- ##
============================================
+ Coverage 72.09% 72.67% +0.57%
- Complexity 65 90 +25
============================================
Files 695 705 +10
Lines 32018 32728 +710
Branches 2714 2809 +95
============================================
+ Hits 23084 23784 +700
+ Misses 7694 7638 -56
- Partials 1240 1306 +66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5a22288 to
36cb74a
Compare
36cb74a to
4571c36
Compare
4571c36 to
93f049a
Compare
3f4b1fc to
ede0363
Compare
| - discovery.type=single-node | ||
| - DISABLE_SECURITY_PLUGIN=true | ||
| - OPENSEARCH_INITIAL_ADMIN_PASSWORD=Admin_1234! | ||
| - OPENSEARCH_JAVA_OPTS=-Xms256m -Xmx256m |
There was a problem hiding this comment.
Why did we need to modify the existing OS container definition?
There was a problem hiding this comment.
I added it because running two OpenSearch instances (destination + reporting) caused the destination to get OOM killed. But no need to do for the destination, so fixed it by removing the heap limit from the destination OS container and set the reduced heap on the reporting node.
There was a problem hiding this comment.
@Shivani-techno take a look at your config in docker desktop for cpu/memory allocated to the docker runner, it may have caused the OOM you were seeing
| - "18080:8080" | ||
| volumes: | ||
| - transform-dist:/transforms:ro | ||
| - ../TrafficCapture/SolrTransformations/docker/reporting-config.yaml:/config/reporting-config.yaml:ro |
There was a problem hiding this comment.
Please create a reporting config for this harness specifically. The one in the other directory should serve only as a template.
| - opensearch=request:/transforms/solr-to-opensearch-request.js,response:/transforms/solr-to-opensearch-response.js | ||
| - --watchTransforms | ||
|
|
||
| # Mode 3: Dual-target, Solr primary — returns Solr response, validates against OpenSearch |
There was a problem hiding this comment.
Since we are modifying the test harness (aka. dev sandbox) we should not modify this file at all.
There was a problem hiding this comment.
done, reverted back the changes
ede0363 to
cb2dd17
Compare
5088b3b to
920f22c
Compare
920f22c to
31122cd
Compare
…porting node and Dashboards Signed-off-by: Shivani - <scivani@amazon.com>
31122cd to
eb45cee
Compare
| retries: 30 | ||
|
|
||
| opensearch-reporting: | ||
| image: opensearchproject/opensearch:3.3.0 |
| - discovery.type=single-node | ||
| - DISABLE_SECURITY_PLUGIN=true | ||
| - OPENSEARCH_INITIAL_ADMIN_PASSWORD=Admin_1234! | ||
| - OPENSEARCH_JAVA_OPTS=-Xms256m -Xmx256m |
There was a problem hiding this comment.
Bump this to at least 1gb on Xmx, we've had issues with smaller opensearch clusters
AndreKurait
left a comment
There was a problem hiding this comment.
Please take a look at the tuple interface in the replayer. See #2605 which can be leveraged for high performance validation here.
| * - headers use dynamic mapping since HTTP header values are always strings | ||
| * and individual headers vary per request | ||
| */ | ||
| private String buildIndexTemplateJson() { |
There was a problem hiding this comment.
Can we move this to preflight step prior to the shim starting up
| private String buildIndexTemplateJson() { | ||
| return String.format(""" | ||
| { | ||
| "index_patterns": ["%s-*"], |
There was a problem hiding this comment.
Do we have any test in here that validates if this works against the reporting opensearch version?
| if (buffer.size() >= bulkSize) { | ||
| List<ValidationDocument> batch = new ArrayList<>(buffer); | ||
| buffer.clear(); | ||
| scheduler.execute(() -> sendBulk(batch)); |
There was a problem hiding this comment.
This is going to be a bottleneck by single threading submit and waiting on the bulk request.
| try { | ||
| synchronized (buffer) { | ||
| buffer.add(document); | ||
| if (buffer.size() >= bulkSize) { |
There was a problem hiding this comment.
Depending on number of documents without consideration for their size is troublesome. E.g. if each doc was 20MB this would be a 2GB request which opensearch would reject
Signed-off-by: Shivani - <scivani@amazon.com>
AndreKurait
left a comment
There was a problem hiding this comment.
This comment serves so i can use "Changes since reivew" post force-push's
…emplate, improve coverage Signed-off-by: Shivani - <scivani@amazon.com>
Signed-off-by: Shivani - <scivani@amazon.com>
Signed-off-by: Shivani - <scivani@amazon.com>
Signed-off-by: Shivani - <scivani@amazon.com>
Signed-off-by: Shivani - <scivani@amazon.com>
| public class OpenSearchMetricsSink implements MetricsSink { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(OpenSearchMetricsSink.class); | ||
| private static final ObjectMapper MAPPER = new ObjectMapper(); |
There was a problem hiding this comment.
There should be a mapper factory we should use
| } | ||
| } | ||
|
|
||
| private long estimateDocSize(ValidationDocument document) { |
There was a problem hiding this comment.
Note, we have existing code in this project to do this without reserializing. See how RFS generates bulk documents
| ndjson.append(MAPPER.writeValueAsString(doc)).append("\n"); | ||
| } | ||
|
|
||
| var requestBuilder = HttpRequest.newBuilder() |
There was a problem hiding this comment.
We have existing code that does this in a robust way (e.g. applying GZIP client compression) ideally we would use the same underlying code for performance and maintainability
| .timeout(Duration.ofSeconds(30)); | ||
|
|
||
| if (authHeader != null) { | ||
| requestBuilder.header("Authorization", authHeader); |
There was a problem hiding this comment.
Looks like this only supports basic auth, if you used our existing construcs in replay or RFS this could support sigv4
| * Checks for partial failures in bulk response. | ||
| * Even if HTTP status is 200, individual documents may have failed. | ||
| */ | ||
| void checkPartialFailures(String responseBody, int totalDocs) { |
There was a problem hiding this comment.
We already have exisitng logic in RFS which does this as well as a bisect on failure to only retry the failed docs, could be useful here
| public boolean watchTransforms; | ||
|
|
||
| @Parameter(names = {"--reporting-config"}, | ||
| description = "Path to YAML configuration file for the validation reporting framework.") |
There was a problem hiding this comment.
are we getting value for the complexity of YAML here when the rest of the processes support JSON natively?
| * Validates index template creation, document indexing, and mapping against a real OpenSearch instance. | ||
| */ | ||
| @Testcontainers | ||
| @Tag("longTest") |
There was a problem hiding this comment.
Use isolatedTest for testcontainers
| .connectTimeout(Duration.ofSeconds(10)).build(); | ||
|
|
||
| @Container | ||
| static final OpensearchContainer<?> opensearch = new OpensearchContainer<>("opensearchproject/opensearch:2.19.1") |
There was a problem hiding this comment.
Please see our helper methods for test containers which we use across our processes. Why is this using opensearch 2.19?
…config to JSON Signed-off-by: Shivani - <scivani@amazon.com>
|
Checking in, are there more changes expected here ? Should we move this PR into draft until these changes are made ? |
Description
Adds explicit index template creation to OpenSearchMetricsSink so validation metrics
indices have consistent, optimized field mappings instead of relying on dynamic mapping.
Changes:
types (keyword for aggregation fields, date for timestamp, nested for comparisons,
text for request bodies with no length limit)
to collect and publish validation metrics after each request
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.