Skip to content
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.

Sign up for GitHub

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

Additional metrics collection #3099

Merged
merged 8 commits into from
Sep 11, 2024
Merged

Additional metrics collection #3099

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a4cd361
Additional metrics collection
mullermp Sep 6, 2024
70351d5
Fix failing tests
mullermp Sep 6, 2024
5e8cafd
Fix typo
mullermp Sep 6, 2024
0cdb3dd
Move sigv4a metric to endpoints plugin
mullermp Sep 6, 2024
856caf2
Use regional endpoint config for setting metric
mullermp Sep 6, 2024
b5eba79
PR feedback
mullermp Sep 11, 2024
43d2f21
Merge branch 'version-3' into extra-metrics
mullermp Sep 11, 2024
d0f22b2
PR feedback
mullermp Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/endpoints_module.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ def initialize(options)

# @return [Array<EndpointParameter>]
attr_reader :parameters

def has_endpoint_built_in?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if Endpoint isn't guaranteed to exist, I think it's still functionally equivalent, since the switch case looks for Endpoint anyway.

parameters.any? { |p| p.param_data['builtIn'] == 'SDK::Endpoint' }
end
end

class EndpointParameter
Expand Down Expand Up @@ -155,7 +151,7 @@ def built_in_client_context_param_value(param_data)
when 'AWS::S3::DisableMultiRegionAccessPoints'
'context.config.s3_disable_multiregion_access_points'
when 'SDK::Endpoint'
'endpoint'
'context.config.regional_endpoint ? nil : context.config.endpoint.to_s'
end
end

Expand Down
13 changes: 7 additions & 6 deletions ...-sdk-code-generator/lib/aws-sdk-code-generator/views/spec/endpoint_provider_spec_class.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ def initialize(options)
operation_name: Underscore.underscore(
operation_inputs_test['operationName']
),
operation_params: operation_inputs_test['operationParams'] || [],
built_in_params: operation_inputs_test['builtInParams'] || [],
client_params: operation_inputs_test['clientParams'] || []
operation_params: operation_inputs_test['operationParams'] || {},
built_in_params: operation_inputs_test['builtInParams'] || {},
client_params: operation_inputs_test['clientParams'] || {}
)
end
end
Expand Down Expand Up @@ -117,12 +117,13 @@ def initialize(options)
@client_params = options[:client_params].map do |k,v|
Param.new(Underscore.underscore(k), v)
end

@client_params += options[:built_in_params].map do |k,v|
built_in_to_param(k, v)
end
# the expected default of UseGlobalEndpoint does not match the SDK's default value
if @service.identifier == 's3' && !options[:built_in_params].include?('AWS::S3::UseGlobalEndpoint')
# the expected default of UseGlobalEndpoint in rules
# does not match the Ruby SDK's default value
if @service.identifier == 's3' &&
!options[:built_in_params].include?('AWS::S3::UseGlobalEndpoint')
@client_params << built_in_to_param('AWS::S3::UseGlobalEndpoint', false)
end
end
Expand Down
5 changes: 0 additions & 5 deletions build_tools/aws-sdk-code-generator/templates/endpoints_module.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ module {{module_name}}
{{#endpoint_classes}}
class {{name}}
def self.build(context)
{{#has_endpoint_built_in?}}
unless context.config.regional_endpoint
endpoint = context.config.endpoint.to_s
end
{{/has_endpoint_built_in?}}
{{module_name}}::EndpointParameters.new(
{{#parameters}}
{{#static_string?}}
Expand Down
18 changes: 17 additions & 1 deletion build_tools/aws-sdk-code-generator/templates/endpoints_plugin.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find these a little clunky. Two thoughts on making it a little easier -

  1. 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).

  2. If we do not want to support multiple places/scopes metrics can be added, then we can have with_metric take an array of metrics, and allow that array to be empty. That will let us easily support conditional metrics and multiple ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with_sigv4a_metric(context[:auth_scheme]) do
@handler.call(context)
end
end
end

private

def with_endpoint_metric(config, &block)
return block.call if config.regional_endpoint

Aws::Plugins::UserAgent.metric('ENDPOINT_OVERRIDE', &block)
end

def with_sigv4a_metric(auth_scheme, &block)
return block.call unless auth_scheme && auth_scheme['name'] == 'sigv4a'

Aws::Plugins::UserAgent.metric('SIGV4A_SIGNING', &block)
end

def apply_endpoint_headers(context, headers)
headers.each do |key, values|
value = values
Expand Down
2 changes: 1 addition & 1 deletion build_tools/aws-sdk-code-generator/templates/spec/endpoint_provider_spec_class.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module {{module_name}}
subject { {{module_name}}::EndpointProvider.new }

{{#endpoint_tests}}
context '{{documentation}}' do
context "{{{documentation}}}" do
mullermp marked this conversation as resolved.
Show resolved Hide resolved
let(:expected) do
{{{expect}}}
end
Expand Down
4 changes: 2 additions & 2 deletions build_tools/services.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ class ServiceEnumerator
MANIFEST_PATH = File.expand_path('../../services.json', __FILE__)

# Minimum `aws-sdk-core` version for new gem builds
MINIMUM_CORE_VERSION = "3.203.0"
MINIMUM_CORE_VERSION = "3.203.1"

# Minimum `aws-sdk-core` version for new S3 gem builds
MINIMUM_CORE_VERSION_S3 = "3.203.0"
MINIMUM_CORE_VERSION_S3 = "3.203.1"

EVENTSTREAM_PLUGIN = "Aws::Plugins::EventStreamConfiguration"

Expand Down
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Additional metrics collection in the User-Agent plugin.

3.203.0 (2024-09-03)
------------------

Expand Down
1 change: 1 addition & 0 deletions gems/aws-sdk-core/lib/aws-sdk-core/plugins/regional_endpoint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ def handle_legacy_pseudo_regions(cfg)
cfg.override_config(:region, new_region)
end
end

# set a default endpoint in config using legacy (endpoints.json) resolver
def resolve_legacy_endpoint(cfg)
endpoint_prefix = cfg.api.metadata['endpointPrefix']
Expand Down
5 changes: 4 additions & 1 deletion gems/aws-sdk-core/lib/aws-sdk-core/plugins/user_agent.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ class UserAgent < Seahorse::Client::Plugin
"S3_CRYPTO_V2": "I",
"S3_EXPRESS_BUCKET": "J",
"S3_ACCESS_GRANTS": "K",
"GZIP_REQUEST_COMPRESSION": "L"
"GZIP_REQUEST_COMPRESSION": "L",
"PROTOCOL_RPC_V2_CBOR": "M",
"ENDPOINT_OVERRIDE": "N",
"SIGV4A_SIGNING": "S"
}
METRICS

Expand Down
6 changes: 5 additions & 1 deletion gems/aws-sdk-core/lib/aws-sdk-core/rpc_v2/handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@ class Handler < Seahorse::Client::Handler
# @return [Seahorse::Client::Response]
def call(context)
build_request(context)
response = @handler.call(context)
response = with_metric { @handler.call(context) }
response.on(200..299) { |resp| resp.data = parse_body(context) }
response.on(200..599) { |_resp| apply_request_id(context) }
response
end

private

def with_metric(&block)
Aws::Plugins::UserAgent.metric('PROTOCOL_RPC_V2_CBOR', &block)
end

def build_request(context)
context.http_request.headers['smithy-protocol'] = 'rpc-v2-cbor'
context.http_request.http_method = 'POST'
Expand Down
14 changes: 0 additions & 14 deletions gems/aws-sdk-core/spec/aws/plugins/request_compression_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,6 @@ def expect_uncompressed_body(resp, body)
end

context 'request compression operation' do
it 'sets user-agent metric for the operation' do
mullermp marked this conversation as resolved.
Show resolved Hide resolved
resp = client.some_operation(body: uncompressed_body)
metric = Aws::Plugins::UserAgent::METRICS['GZIP_REQUEST_COMPRESSION']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end

it 'compresses the body and sets the content-encoding header' do
resp = client.some_operation(body: uncompressed_body)
expect(resp.context.http_request.headers['Content-Encoding']).to eq('gzip')
Expand Down Expand Up @@ -165,13 +158,6 @@ def expect_uncompressed_body(resp, body)
expect_uncompressed_body(resp, small_body)
end

it 'sets user-agent metric for a streaming operation' do
resp = client.operation_streaming(body: 'body')
metric = Aws::Plugins::UserAgent::METRICS['GZIP_REQUEST_COMPRESSION']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end

it 'compresses a large streaming body' do
large_body = StringIO.new('.' * 16_385)
client.stub_responses(:operation_streaming, -> (context) do
Expand Down
9 changes: 0 additions & 9 deletions gems/aws-sdk-core/spec/aws/plugins/retry_errors_legacy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,6 @@ module Plugins
client = RetryErrorsSvc::Client.new(retry_mode: 'legacy', region: 'us-west-2')
expect(client.handlers.entries.map(&:handler_class)).to include(RetryErrors::LegacyHandler)
end

it 'sets user-agent metric' do
client = RetryErrorsSvc::Client.new(retry_mode: 'legacy', region: 'us-west-2')
stub_request(:post, client.config.endpoint)
resp = client.example_operation
metric = Aws::Plugins::UserAgent::METRICS['RETRY_MODE_LEGACY']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end
end

describe RetryErrors::LegacyHandler do
Expand Down
18 changes: 0 additions & 18 deletions gems/aws-sdk-core/spec/aws/plugins/retry_errors_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,24 +106,6 @@ module Plugins
.to receive(:correct_clock_skew).and_return('true')
expect(client.config.correct_clock_skew).to eq(true)
end

it 'sets user-agent metric for standard' do
client = RetryErrorsSvc::Client.new(retry_mode: 'standard', region: 'us-west-2')
stub_request(:post, client.config.endpoint)
resp = client.example_operation
metric = Aws::Plugins::UserAgent::METRICS['RETRY_MODE_STANDARD']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end

it 'sets user-agent metric for adaptive' do
client = RetryErrorsSvc::Client.new(retry_mode: 'adaptive', region: 'us-west-2')
stub_request(:post, client.config.endpoint)
resp = client.example_operation
metric = Aws::Plugins::UserAgent::METRICS['RETRY_MODE_ADAPTIVE']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end
end

describe RetryErrors::Handler do
Expand Down
7 changes: 0 additions & 7 deletions gems/aws-sdk-s3/spec/plugins/express_session_auth_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,6 @@ module S3
end
client.get_object(bucket: bucket, key: 'key')
end

it 'sets user-agent metric' do
resp = client.get_object(bucket: 'bucket--use1-az2--x-s3', key: 'key')
metric = Aws::Plugins::UserAgent::METRICS['S3_EXPRESS_BUCKET']
expect(resp.context.http_request.headers['User-Agent'])
.to include("m/#{metric}")
end
end

# does not have http checksum trait, but requires a checksum (md5)
Expand Down
Loading