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

[cloud provider] Add AWS EC2 instance id semantic convention #600

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+55 −0
Open

[cloud provider] Add AWS EC2 instance id semantic convention #600

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ae07bd2
[cloud-provider] Add semantic convention for AWS EC2 instance id
mx-psi Dec 12, 2023
d9b4093
Add changelog entry
mx-psi Dec 12, 2023
7d194f7
Apply suggestions from code review
mx-psi Dec 12, 2023
ba53832
make table-generation
mx-psi Dec 12, 2023
81f7aa6
Merge branch 'main' into mx-psi/ec2-instance-id
mx-psi Jan 25, 2024
a72994d
Merge branch 'main' into mx-psi/ec2-instance-id
arminru Feb 2, 2024
70b374d
Merge branch 'main' into mx-psi/ec2-instance-id
mx-psi Feb 5, 2024
6508721
Merge branch 'main' into mx-psi/ec2-instance-id
joaopgrassi Feb 5, 2024
0e71d05
Switch to chloggen
mx-psi Feb 5, 2024
997d4b1
Merge branch 'main' into mx-psi/ec2-instance-id
arminru Feb 6, 2024
c20f25d
Merge branch 'main' into mx-psi/ec2-instance-id
mx-psi Feb 29, 2024
4fbfab3
Merge branch 'main' into mx-psi/ec2-instance-id
joaopgrassi Mar 20, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ release.
([#163](https://github.com/open-telemetry/semantic-conventions/pull/163))
- Add .NET 8.0 metrics for HTTP client, ASP.NET Core, SignalR server and Kestrel.
([#283](https://github.com/open-telemetry/semantic-conventions/pull/283))
- Add `aws.ec2.instance.id` resource attribute for AWS EC2 VMs.
([#600](https://github.com/open-telemetry/semantic-conventions/pull/600))

### Fixes

Expand Down
17 changes: 17 additions & 0 deletions docs/resource/cloud-provider/aws/ec2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# AWS EC2

**Status**: [Experimental][DocumentStatus]

**type:** `aws.ec2`

**Description:** Resources used by Amazon Elastic Compute Cloud (Amazon EC2).

<!-- semconv aws.ec2 -->
| Attribute | Type | Description | Examples | Requirement Level |
|---|---|---|---|---|
| `aws.ec2.instance.id` | string | The instance id of an AWS EC2 instance. This is the value provided by the EC2 Metadata endpoint under `instance-id`. [1] | `i-1234567890abcdef0` | Recommended |

**[1]:** When both `host.id` and `aws.ec2.instance.id` are present, they SHOULD be equal.
<!-- endsemconv -->

[DocumentStatus]: https://github.com/open-telemetry/opentelemetry-specification/tree/v1.26.0/specification/document-status.md
16 changes: 16 additions & 0 deletions model/resource/cloud_provider/aws/ec2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
groups:
- id: aws.ec2
prefix: aws.ec2
Copy link
Contributor

Choose a reason for hiding this comment

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

Please stop using prefix.

type: resource
brief: >
Resources used by Amazon Elastic Compute Cloud (Amazon EC2).
attributes:
- id: instance.id
Copy link
Member

Choose a reason for hiding this comment

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

@mx-psi Can you move the definition of this attribute to the registry and use a reference here? (see https://github.com/open-telemetry/semantic-conventions/pull/434/files for an example)

type: string
brief: >
The instance id of an AWS EC2 instance. This is the value
provided by the EC2 Metadata endpoint under `instance-id`.
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
note: >
When both `host.id` and `aws.ec2.instance.id` are present,
they SHOULD be equal.
Comment on lines +15 to +16
Copy link
Member

Choose a reason for hiding this comment

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

Could this also be phrased as follows to avoid duplication?

Suggested change
When both `host.id` and `aws.ec2.instance.id` are present,
they SHOULD be equal.
When the value in `host.id` is the same value as the `instance-id`
returned by the metadata endpoint, only `host.id` should be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per #576 (comment) I believe this is not what we do in similar cases such as with service.instance.id and k8s.pod.name. I think the situation is very similar here and we should do the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

@arminru PTAL!

Copy link
Member

Choose a reason for hiding this comment

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

It certainly makes the logic for both setting and reading the attribute easier. The duplicated string should in transport be taken care of by compression and also backends storing it would be able to deduplicate it as well.

I don't think we ever made a proper policy decision on whether to favor simplicity (for both instrumentation and querying) or deduplication but I believe either is a viable approach to take.

@open-telemetry/specs-semconv-approvers WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we ever made a proper policy decision on whether to favor simplicity (for both instrumentation and querying) or deduplication but I believe either is a viable approach to take.

fwiw, we abandoned de-duplication in network.peer.address etc (328a2c6) because it made it hard to know whether an attribute is missing because it was a duplicate or because the particular instrumentation doesn't capture it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this moves in a direction I'm hoping to push resource-detection -

That is a division of responsibilities. An AWS detector would be responsible for this attribute but NOT host.id.

Instead a Host Detector would generically detect host attributes as best as possible across known host-lookup mechanisms.

I'm writing up thoughts on this now, but I'd be in favor of this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm writing up thoughts on this now, but I'd be in favor of this PR

@jsuereth Do you have any updates on this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm supportive of the addition as originally proposed by @mx-psi

examples: ['i-1234567890abcdef0']
Loading