-
Notifications
You must be signed in to change notification settings - Fork 102
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
Data stream stats: Add name only if present #338
Data stream stats: Add name only if present #338
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.
Looks good, care to add a test, please?
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.
Thank you @DewaldV! +1 for adding a test for this.
Thanks for taking a look, I'll add a test to this as requested. 👍 Apologies for my delayed response I've been at conference this week. |
@VachaShah @dblock apologies for the delay in getting those tests sorted but I've added them as requested. The integration test is a bit strange since I had to add additional Data Stream create and delete tests so that I could ensure I was actually getting multiple results from the I added a unit test to specifically check the behaviour of setting the |
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.
This is perfect, my favorite kind of PR that has way more tests than an otherwise trivial fix!
@DewaldV see failures in tests + missing license header |
@dblock Apologies that I missed that. Just pushed the license headers! |
Cool. I disabled the PR behavior to require approvals from maintainers to run for 1st time contributors (made it for new GitHub users only) |
Ahh, the test failure I thought might be an issue but I ran a it a few times locally and it seemed OK so I left it as is. I suspect the When I tested locally it seemed consistent but it's likely consistent for my local cluster only compared to running it somewhere else. I'll have to refactor that test to change how we do the assertion so its not order dependent. |
Signed-off-by: DewaldV <dewald.viljoen@pm.me>
We want to test the path of the URI specifically to ensure that we're constructing it correctly. Signed-off-by: DewaldV <dewald.viljoen@pm.me>
We test for multiple data stream stats on the /_stats endpoint. This needs another datastream added/removed to ensure we test it sufficiently. Signed-off-by: DewaldV <dewald.viljoen@pm.me>
Signed-off-by: DewaldV <dewald.viljoen@pm.me>
Signed-off-by: DewaldV <dewald.viljoen@pm.me>
Hey @dblock. 👋 After some research and attempts at fixing the ordering issue in tests I found the jsonassert library that tackles exactly these sorts of issues. It might be a very useful tool for other tests in this lib given the usage of JSON for all OpenSearch APIs. It includes functionality for asserting JSON objects match without considering array order and should fix the flakiness in my original test. I understand introducing a new dependency isn't ideal and might need more conversation but this is only used in Integration Tests at this point so hopefully it can be considered. |
I've checked the failures on the latest CI run and I can't replicate them locally. I'm convinced they are unrelated to my changes and tests and may just need a rerun to pass. I can do some more digging if needed thoug, let me know what you think. 👍 |
Description
This makes the GetDataStreamStats request only add the
Name
and path separator (/
) to the path if Name is not empty.As it stands when using this function without setting a name the path will be:
This path is then used by the AWS Signer to calculate a signature. When the request is sent the server recalculates the signature but the additional
/
has been dropped by now and we get a signature mismatch. See #337 for details.I have tested this fix in our own codebase and it resolves the issue as described.
Issues Resolved
Closes #337
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.