-
Notifications
You must be signed in to change notification settings - Fork 80
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
Improve the telemetry device "Segment Replication Stats" by changing to parse JSON format and using numeric bytes value #356
Improve the telemetry device "Segment Replication Stats" by changing to parse JSON format and using numeric bytes value #356
Conversation
…tespace problem for telemetry - segment replication stats Signed-off-by: Tianli Feng <ftianli@amazon.com>
… API response Signed-off-by: Tianli Feng <ftianli@amazon.com>
849ec67
to
10a1fea
Compare
"shard_id": "[so][0]", | ||
"target_node": "node-1", | ||
"target_host": "127.0.0.1", | ||
"shard_id": "[logs-211998][6]", |
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.
nit: Is shard_id
, target_node
change needed in tests ?
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.
@dreamer-89 Thanks a lot for your review! 👏 The change is indeed not necessary, I just updated the test with a real API response from my recent performance test. I think it's fine to keep the changes.
Signed-off-by: Tianli Feng <ftianli@amazon.com>
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. Thanks for discovering this bug and providing the fix @tlfeng!
@IanHoang Thank you for your review! It's embarrassed that I introduced the bug. |
Description
Although PR #339 added new telemetry device "Segment Replication Stats", there are some problems. The PR will solve the problem.
str.split(" ")
as the delimiter to parse the REST API response, where maybe more than 1 space between adjacent values.For detail, see #339 (comment).
Issues Resolved
Address a problem related to issue #339
The screenshot illustrates the current problem. Sometime when there are more than 1 space between adjacent values, it leads mismatch in the fields and values of the result.
Testing
End to end test:
Verified that every field has correct value.
Command used:
opensearch-benchmark execute-test --workload=http_logs --pipeline=benchmark-only \ --target-hosts=opens-clust-xxx.elb.us-west-2.amazonaws.com:80 \ --include-tasks=delete-index,create-index,check-cluster-health,index-append \ --workload-params='{"index_settings":{"number_of_shards": 1, "number_of_replicas": 1 }}' \ --telemetry=segment-replication-stats --test-mode
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.