-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ASF/CloudWatch: add support for multi-protocols #13161
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
Test Results - Alternative Providers66 tests 38 ✅ 3m 32s ⏱️ Results for commit 0d8f406. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 36m 38s ⏱️ Results for commit 0d8f406. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 1h 58m 53s ⏱️ Results for commit 0d8f406. ♻️ This comment has been updated with latest results. |
00ccd61 to
0f27df7
Compare
51871cc to
61416ea
Compare
0f27df7 to
7e3922b
Compare
alexrashed
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.
Absolutely awesome! The final piece of the puzzle for #13028! It already performs the modifications on the specs which are about to come in the future release (according to the preview build) 🤩
It continues to surprise me how hard "time" can be (there was quite some unexpected stuff with the tz handling for cloudwatch in this PR) 😅, and the moto fallback handling is an interesting case as well. 💯
| """ | ||
| This function is taken from botocore Client._convert_to_request_dict, but we are overriding the serializer | ||
| Botocore does not expose a way to create a client with a specific protocol, but we need this functionality | ||
| to support multi-protocols. | ||
| """ |
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.
thought: Just keeping note of what we discussed yesterday. I think this is an interesting example where a feature like this (manual protocol selection / changing the precedence for a request) could be interesting in botocore. However, we have been using botocore internals here before, so for now I think this approach is just fine. 💯
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.
Yes, definitely agree 💯 hopefully we get to configure that on the client level, for now I'll use patching to run the full test suite 👍
| @markers.aws.validated | ||
| @pytest.mark.parametrize("protocol", ["json", "smithy-rpc-v2-cbor", "query"]) | ||
| @markers.snapshot.skip_snapshot_verify( | ||
| paths=[ | ||
| # LogAlarms is not defined in the Boto specs anywhere, but is returned in raw responses (deprecated?) | ||
| "$.describe-alarms..LogAlarms", | ||
| ] | ||
| ) |
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.
praise: 🥳 This is the test which actually runs against all 3 protocols! 🤩
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.
praise: This is basically patching the existing botocore specs with what the preview builds of botocore for this feature are doing. 💯
| action=context.operation.name, | ||
| parameters=service_request, | ||
| region=context.region, | ||
| protocol=context.protocol, |
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.
thought (non-blocking): That's an interesting one - as you described in your PR description - as it won't work for CBOR protocols with moto just yet. For now I definitely agree that we should not over-complicate this here, especially since:
- moto might add support at some point
- protocols are changing slowly, and cbor isn't widely used yet
- for the services using cbor it's really only our cloudwatch v1 provider which falls back to moto, and imho we could delete this old legacy provider already
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.
Yes, that's true. It will also be the case for all regular call_moto, as we don't re-serialize the request there.
I'd also be a bit worried about changing the overall format of the request to be able to pass it to 3rd party backend, as it could lead to unexpected behavior as well (meaning going rpc-v2-cbor -> json -> parse json back -> serialize rpc-v2-cbor)
But good to be aware of it 👍
e28e8fe to
0d8f406
Compare
Motivation
TBD, stacked on of top #13151
This PR is part of a bigger initiative (see #13028) to implement support for the new Smithy Protocol
smithy-rpc-v2-cbor, as well as the new Botocore multi-protocol support for single services (before, a service could only define one protocol).This PR implements the new multi-protocol support for the CloudWatch service, by already implementing the future spec for the service as defined in https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch-protocols-faq.html
The biggest part of this PR is the testing of the new specs: we will implement full multi-protocol testing of our entire CloudWatch test suite in a follow-up, but we are already testing that it can handle all 3 protocols (
query,jsonandsmithy-rpc-v2-cbor) by making requests via a plain HTTP client.We also needed to update our dispatchers to 3rd party backends like Moto to be protocol-aware so that they would re-serialize and parse the incoming HTTP response properly based on the current request protocol. This meant some changes in signature of
create_aws_request_contextandparse_response.For example, for the legacy v1 provider relying on Moto, CloudWatch with
smithy-rpc-v2-cbor, we will get anInternalErrorfrom moto withValueError: Bad status: 501, response body: {'__type': 'InternalFailure', 'message': 'Unsupported protocol [smithy-rpc-v2-cbor] for service cloudwatch'}Note: I don't think we should try to adapt ourselves to what Moto supports, because there is no way to know from the specs what was the previous "default" protocol, so we would need to maintain a hardcoded list for those.
While implementing those tests, a few small issues came to light:
ArcRegionSwitchdoes not define timestamp input)Valuesasintinstead offloatinGetMetricDataparse_timestamputility returned naive datetime objectsNote: current test failure is unrelated, will be fixed when new Pro version will be out
Changes
cloudwatchand add support for multi protocolGetMetricDatatype ofValuesparse_timestamputility only used in S3 notifications and CloudTrail which was returning naive datetime even though the timezone data was encoded and should be used (put into light after the timestamp parsing of JSON)Testing
I've also tested that it works with the beta version of the AWS CLI / botocore / boto3 that is available at the CloudWatch protocol FAQ
I followed the steps as given in the linked issue:
pip install boto3-1.39.7-py3-none-any.whl botocore-1.39.7-py3-none-any.whl awscli-1.41.7-py3-none-any.whlAWS_ENDPOINT_URL=http://localhost:4566 AWS_ACCESS_KEY_ID=test AWS_SECRET_ACCESS_KEY=test AWS_REGION=us-east-1 python -m awscli cloudwatch describe-alarms --debugjsonprotocol by checking itsContent-Typeor itsCanonicalRequestin the debug logs: