Skip to content

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Sep 17, 2025

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, json and smithy-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_context and parse_response.
For example, for the legacy v1 provider relying on Moto, CloudWatch with smithy-rpc-v2-cbor, we will get an InternalError from moto with ValueError: 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:

  • when parsing UNIX timestamp, we would return naive datetime objects, however for other kind of timestamp format we'd return timezone aware object. This was an issue, and it is fixed in this PR
  • when parsing Timestamp with our CBOR parser, the datetime could sometimes already be parsed so we shouldn't try to convert it twice (this couldn't be fixed before because ArcRegionSwitch does not define timestamp input)
  • when serializing empty responses with our CBOR serializer, we would fail as we were looking at a non-existing shape
  • CloudWatch would return Values as int instead of float in GetMetricData
  • the parse_timestamp utility returned naive datetime objects

Note: current test failure is unrelated, will be fixed when new Pro version will be out

Changes

  • implement the future spec for cloudwatch and add support for multi protocol
  • add test suite to verify it works with all 3 protocols
  • add additional small fixes:
    • fix naive datetime object parsing for unix timestamp
    • fix timestamp parsing in CBOR parser
    • fix serializing empty response in CBOR serializer
    • fix GetMetricData type of Values
    • update dispatchers to be protocol aware and update changed signatures in tests and usage
    • fix the parse_timestamp utility 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:

  • checkout the PR and run it in host mode
  • go to the downloaded folder from the link above
  • create a venv and activate it
  • run 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.whl
  • run AWS_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 --debug
  • verify that the request used the json protocol by checking its Content-Type or its CanonicalRequest in the debug logs:
     POST
     /
     
     content-type:application/x-amz-json-1.0
     host:localhost:4566
     x-amz-date:20250917T222501Z
     x-amz-target:GraniteServiceVersion20100801.DescribeAlarms
     x-amzn-query-mode:true
    

@bentsku bentsku added this to the 4.9 milestone Sep 17, 2025
@bentsku bentsku self-assigned this Sep 17, 2025
@bentsku bentsku added aws:cloudwatch Amazon CloudWatch area: asf semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases docs: skip Pull request does not require documentation changes labels Sep 17, 2025
@github-actions
Copy link

github-actions bot commented Sep 17, 2025

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0    2 suites  ±0   7m 45s ⏱️ -19s
  532 tests ±0  480 ✅ ±0   52 💤 ±0  0 ❌ ±0 
1 064 runs  ±0  960 ✅ ±0  104 💤 ±0  0 ❌ ±0 

Results for commit 0d8f406. ± Comparison against base commit f188c62.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Test Results - Preflight, Unit

22 277 tests  +82   20 539 ✅ +82   17m 14s ⏱️ + 2m 12s
     1 suites ± 0    1 738 💤 ± 0 
     1 files   ± 0        0 ❌ ± 0 

Results for commit 0d8f406. ± Comparison against base commit f188c62.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 19s ⏱️ -1s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 0d8f406. ± Comparison against base commit f188c62.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Test Results - Alternative Providers

66 tests   38 ✅  3m 32s ⏱️
 1 suites  28 💤
 1 files     0 ❌

Results for commit 0d8f406.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 36m 38s ⏱️
5 041 tests 4 559 ✅ 482 💤 0 ❌
5 047 runs  4 559 ✅ 488 💤 0 ❌

Results for commit 0d8f406.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

LocalStack Community integration with Pro

    2 files      2 suites   1h 58m 53s ⏱️
4 667 tests 4 345 ✅ 322 💤 0 ❌
4 669 runs  4 345 ✅ 324 💤 0 ❌

Results for commit 0d8f406.

♻️ This comment has been updated with latest results.

@bentsku bentsku force-pushed the cloudwatch-multi-protocol branch 2 times, most recently from 00ccd61 to 0f27df7 Compare September 18, 2025 02:17
@bentsku bentsku force-pushed the cloudwatch-multi-protocol branch from 0f27df7 to 7e3922b Compare September 18, 2025 13:57
@bentsku bentsku marked this pull request as ready for review September 18, 2025 16:04
@bentsku bentsku marked this pull request as draft September 18, 2025 16:05
Copy link
Member

@alexrashed alexrashed left a 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. 💯

Comment on lines +300 to +304
"""
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.
"""
Copy link
Member

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. 💯

Copy link
Contributor Author

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 👍

Comment on lines +2951 to +2958
@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",
]
)
Copy link
Member

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! 🤩

Copy link
Member

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,
Copy link
Member

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

Copy link
Contributor Author

@bentsku bentsku Sep 19, 2025

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 👍

Base automatically changed from asf-multi-protocol to main September 19, 2025 09:34
@bentsku bentsku force-pushed the cloudwatch-multi-protocol branch from e28e8fe to 0d8f406 Compare September 19, 2025 09:37
@bentsku bentsku marked this pull request as ready for review September 19, 2025 09:40
@bentsku bentsku merged commit 0809063 into main Sep 19, 2025
49 checks passed
@bentsku bentsku deleted the cloudwatch-multi-protocol branch September 19, 2025 12:17
@alexrashed alexrashed added the notes: needed Pull request should be mentioned in the release notes label Sep 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: asf aws:cloudwatch Amazon CloudWatch docs: skip Pull request does not require documentation changes notes: needed Pull request should be mentioned in the release notes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants