Additional metrics collection #3099
Conversation
build_tools/aws-sdk-code-generator/templates/spec/endpoint_provider_spec_class.mustache
Show resolved
Hide resolved
| @@ -42,10 +42,6 @@ def initialize(options) | |||
|
|
|||
| # @return [Array<EndpointParameter>] | |||
| attr_reader :parameters | |||
|
|
|||
| def has_endpoint_built_in? | |||
There was a problem hiding this comment.
Why is this being removed?
There was a problem hiding this comment.
I have this in the account id change that was reviewed by us .. but I decided to carry it forward here - it's really just a syntax change. Is it incorrect? I think Endpoint is ALWAYS present in rules is it not? In the switch case below, it is switching on built in, and on SDK::Endpoint, we do the same check that was previously in the mustache template - check regional endpoint and either nil or endpoint.
There was a problem hiding this comment.
I don't think it MUST be present in the rules, but the code we have for constructing the SDK::Endpoint parameter is already conditional, so I agree this is fine, I just wanted to check.
There was a problem hiding this comment.
Even if Endpoint isn't guaranteed to exist, I think it's still functionally equivalent, since the switch case looks for Endpoint anyway.
| @@ -44,11 +44,27 @@ module {{module_name}} | |||
| context[:auth_scheme] = | |||
| Aws::Endpoints.resolve_auth_scheme(context, endpoint) | |||
|
|
|||
| @handler.call(context) | |||
| with_endpoint_metric(context.config) do | |||
There was a problem hiding this comment.
I find these a little clunky. Two thoughts on making it a little easier -
-
For many metric cases (like these), its scoped within a single request. We can add these metrics to the context instead of to a thread local. That way, they do not need to a call a block and pop the metric off. This both lets do conditional metrics more easily and avoids having to wrap the handler call, which isn't really necessary for these. (Note: I'm suggesting a second way to set metrics - scoped only to the request. Not replacing the thread local, which I know we need for other cases).
-
If we do not want to support multiple places/scopes metrics can be added, then we can have
with_metrictake an array of metrics, and allow that array to be empty. That will let us easily support conditional metrics and multiple ones.
There was a problem hiding this comment.
Yeah, it's pretty clunky especially as these add. I think we want to keep a single mechanism (thread local) but probably this can just take an array and we build the array and block call with it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the
CHANGELOG.mdfile (at corresponding gem). For the description entry, please make sure it lives in one line and starts withFeatureorIssuein the correct format.For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md
Thank you for your contribution!