Env Detection

[JonathanHenson]

For p4d.24xlarge:

Is on Ec2 Instance nitro ? True
Instance Type = p4d.24xlarge
Is Optimized for System = True

For trn1n.32xlarge:

Is on Ec2 Instance nitro ? True
Instance Type = trn1n.32xlarge
Is Optimized for System =  True

For p5.48xlarge:

Is on Ec2 Instance nitro ? True
Instance Type = p5.48xlarge
Is Optimized for System = True

For c5n.18xlarge:

Is on Ec2 Instance nitro ? True
Instance Type = c5n.18xlarge
Is Optimized for System = False

For t2.xlarge

Is on Ec2 Instance nitro ? {} False
Instance Type = {} None
Is Optimized for System = {} False

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

[graebm]

trivial/style: we always call this variable _binding

Suggested change

	self._env = _awscrt.load_system_environment()
	self._binding = _awscrt.load_system_environment()

[JonathanHenson]

just going to pull it and cache internally

[graebm]

trivial: I've never figured out how to make python compile its C-extensions in debug mode, so these asserts are pointless

remove them, or use AWS_FATAL_ASSERT

[graebm]

I got confused when the function names changed at the different layers. We usually have them match the underlying C function

Suggested change

	AWS_PY_METHOD_DEF(load_system_environment, METH_NOARGS),
	AWS_PY_METHOD_DEF(is_env_ec2, METH_VARARGS),
	AWS_PY_METHOD_DEF(system_environment_load, METH_NOARGS),
	AWS_PY_METHOD_DEF(system_environment_is_running_on_ec2, METH_VARARGS),

[graebm]

Why mention "nitro" here, but not in the underlying C function?

I liked the is_running_on_ec2 name better, FWIW

[JonathanHenson]

It's really a nitro check, and ec2 is implied as a result.

I'll make sure nitro is put in there and fuse it with the suggestion you made

[graebm]

This is awkward:
system_env.is_crt_s3_optimized_for_system_env()

flip the words around to sound more natural:
system_env.is_optimized_for_crt_s3()

or it might feel more natural as a separate function in s3.py:
awscrt.s3.is_optimized_for_system(system_env)

[graebm]

Or just have 1 public function: awscrt.s3.is_optimized_for_system()

and don't bother binding the aws_system_environment at all, just keep it as an implementation detail

we can have python cache the answer, so it only does work the first time it's called

[JonathanHenson]

That's fine. I added verbosity here largely for aid in debugging when something goes wrong, we can at least ask the customer to add a python function to their script and it's much simpler to track down, than having to peel through logs or c source.

[graebm]

the underylying C function is get_virtualization_product_name()

let's consistently call it "ec2" or "virtualization_product_name" but it's annoying to change terminology between the layers

[JonathanHenson]

ec2 and vritualization product name are not the same thing. ec2 is A vendor, and aws-c-common is mostly generic, I did move most of this to aws-c-s3 however and it makes more sense there. I can clean it up here.

[graebm]

Inherit from NativeResource. we just do this to help tests track leaks

Suggested change

	class SystemEnvironment:
	def __init__(self):
	class SystemEnvironment(NativeResource):
	def __init__(self):
	super().__init__()

[JonathanHenson]

I'm just gonna pull out systemenvironment entirely and cache it internally as you suggested above.