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.

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

UserAgent Updates #414

Merged
merged 17 commits into from
Mar 22, 2024
Merged

UserAgent Updates #414

merged 17 commits into from
Mar 22, 2024

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Mar 19, 2024

Description of changes:
Adds platform/ec2InstanceType if available to the user agent header. Here is new header value:

 UserAgentHeader:CRTS3NativeClient/0.1.x platform/c5n.18xlarge

or

 UserAgentHeader:CRTS3NativeClient/0.1.x platform/unknown

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.37%. Comparing base (e91577e) to head (e999e23).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #414   +/-   ##
=======================================
  Coverage   89.37%   89.37%           
=======================================
  Files          20       20           
  Lines        5854     5866   +12     
=======================================
+ Hits         5232     5243   +11     
- Misses        622      623    +1     
Files Coverage Δ
source/s3.c 96.00% <100.00%> (+0.34%) ⬆️
source/s3_platform_info.c 45.69% <100.00%> (+0.36%) ⬆️
source/s3_util.c 98.08% <100.00%> (+0.05%) ⬆️

... and 1 file with indirect coverage changes

source/s3_util.c Outdated

const size_t user_agent_product_version_length =
g_user_agent_header_product_name.len + forward_slash.len + g_s3_client_version.len;
struct aws_byte_cursor platform_cursor = aws_s3_get_current_platform_info()->instance_type;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussion point: This call can result in an imds call for one time. Should we define a new API which returns instance type only if we know it already?

Copy link
Contributor Author

@waahm7 waahm7 Mar 20, 2024

Choose a reason for hiding this comment

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

I have updated it to only use cached results.

@waahm7 waahm7 changed the title WIP | UserAgent Updates UserAgent Updates Mar 20, 2024
@waahm7 waahm7 marked this pull request as ready for review March 20, 2024 18:01
Comment on lines 560 to 564
if (!cached_only) {
aws_s3_get_ec2_instance_type(loader);
}
/* will never be mutated after the above call. */
return &loader->lock_data.current_env_platform_info;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmmm, we are touching the lock_data. I haven't dig deep into the implementation, but the comment says "will never be mutated after the above call", but, we can now skip aws_s3_get_ec2_instance_type, will it be a possible race condition?

Copy link
Contributor Author

@waahm7 waahm7 Mar 21, 2024

Choose a reason for hiding this comment

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

Thanks in theory, yes. In practice, this would not have occurred since the flow is that this API is called first to determine target_throughput, and the S3 client is created later with that target_throughput. I have updated the method to use an API with locks instead, since it can have a race condition in theory and in practice in the future.

@waahm7 waahm7 merged commit efeb625 into main Mar 22, 2024
30 checks passed
@waahm7 waahm7 deleted the user-agent-pop branch March 22, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants