-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[processor/resourcedetectionprocessor] Introduce instance life cycle to AWS EC2 resource attributes #40191
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
base: main
Are you sure you want to change the base?
Conversation
@@ -8,6 +8,7 @@ | |||
|
|||
| Name | Description | Values | Enabled | | |||
| ---- | ----------- | ------ | ------- | | |||
| aws.ec2.instance_life_cycle | The EC2 instance life cycle | Any Str | true | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a state like attribute? I wonder if that should be a metric then 🤔 .
See open-telemetry/semantic-conventions#1554.
Probably discussing this and defining this first with Semantic Conventions project would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the name is confusing, it's actually indicates the EC2 billing model, values can be spot
, on-demand
or reserved
.
I did some research on Semantic Conventions trying to find a place for it but failed. Two possible namespaces
cloud
, as this attribute is virtual machine specific, butcloud
is a generic namespace for any kind of cloud resources, maybe not the best place.host
, I guess this one has nothing to do with cloud services, maybe not here.
Therefore, I think maybe we can add a non-standard attribute like what they did for ECS or many other resources, check it out here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/resourcedetectionprocessor/internal/aws/ecs/documentation.md?plain=1#L11-L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrsMark does it make sense to you, can we move forward this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank's for the context! I agree that if this attribute cannot be generalised then cloud.*
is not appropriate and it should be under aws.*
namespace. Could you file an issue in Semantic Conventions for this so it can be moved forward?
@dashpole @Aneurysm9 please share your input as code-owners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the issue has been created. open-telemetry/semantic-conventions#2300. Apart from aws.ec2.instance_life_cycle
, I've also added aws.ec2.vpc_id
in the proposal, implementation PR will be created later.
BTW, is e2e required for this change? I've check the test code, since most of them are generated, I'm not sure if I sure do some e2e test, if so where can I find the guideline how to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, is e2e required for this change? I've check the test code, since most of them are generated, I'm not sure if I sure do some e2e test, if so where can I find the guideline how to do it?
I don't think we have e2e tests for this component so probably this addition won't need any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the issue has been created. open-telemetry/semantic-conventions#2300. Apart from aws.ec2.instance_life_cycle, I've also added aws.ec2.vpc_id in the proposal, implementation PR will be created later.
Thank's! @dashpole @Aneurysm9 please consider this too as part of this enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @Aneurysm9 for changes related to AWS resource detectors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can consider instance_life_cycle
as an attribute since other cloud providers also have similar pricing models? For example Azure has pay as you go, that would be similar to the on-demand for AWS.
Description
It's useful to know the EC2 instance life cycle, for example
spot
,on-demand
orreserved
. Especially for costing estimation purpose.As it's not part of semantic conventions, it was added to customized namespace
aws.ec2
like what we have for AWS ECSaws.ecs
.We'll be tracking changes to entitiy attributes for k8s in this SemConv issue.
Testing
Has tested the metadata endpoint of EC2.
Documentation
Added new attribute to AWS EC2 resource attributes document. It's auto generated.