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 all 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
21 changes: 21 additions & 0 deletions .chloggen/mx-psi_ec2-instance-id.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Use this changelog template to create an entry for release notes.
#
# If your change doesn't affect end users you should instead start
# your pull request title with [chore] or use the "Skip Changelog" label.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the area of concern in the attributes-registry, (e.g. http, cloud, db)
component: resource/cloud-provider

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add `aws.ec2.instance.id` resource attribute for AWS EC2 VMs.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [600]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
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](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instancedata-data-categories.html) 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
17 changes: 17 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,17 @@
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](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instancedata-data-categories.html)
endpoint under `instance-id`.
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