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

Add k8s.node in semantic conventions #1341

Closed
wants to merge 3 commits into from

Conversation

XSAM
Copy link
Member

@XSAM XSAM commented Jan 14, 2021

Resolve #1334

@XSAM XSAM requested review from a team January 14, 2021 07:18
@XSAM
Copy link
Member Author

XSAM commented Jan 14, 2021

It looks like make table-generation made lots of changes.

@Oberon00
Copy link
Member

I think it would be important to first clarify what we want to have in host.name. Probably the hostname that the app itself sees when calling gethostname, etc. which would be the hostname of the innermost container in the innermost VM.

@Oberon00
Copy link
Member

It looks like make table-generation made lots of changes.

@XSAM This was just fixed in #1342

| Attribute | Type | Description | Examples | Required |
|---|---|---|---|---|
| `k8s.node.name` | string | The name of the node. | `opentelemetry-node` | No |
| `k8s.node.ip` | string | The address of the node (dotted decimal for IPv4 or [RFC5952](https://tools.ietf.org/html/rfc5952) for IPv6. | `127.0.0.1` | No |
Copy link
Member

@tigrannajaryan tigrannajaryan Jan 14, 2021

Choose a reason for hiding this comment

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

Is this the external IP or internal IP? (see https://kubernetes.io/docs/concepts/architecture/nodes/#addresses). It may be worth explicitly using k8s.node.external_ip and k8s.node.internal_ip instead.

Copy link
Member Author

@XSAM XSAM Jan 15, 2021

Choose a reason for hiding this comment

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

You're right; explicitly declare the type of address might better.

Besides, I found k8s may have multiple ExternalIP and InternalIP. Maybe we should use k8s.node.external_ipsand k8s.node.internal_ips and use string[] as type instead.

For instance: ['127.0.0.1', '127.0.0.2']

Copy link
Member

Choose a reason for hiding this comment

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

@XSAM can you give me a link to a doc which says there may be multiple ExternalIP and InternalIP? I haven't been able to find this in Kubernetes docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to find any documentation specifically about multiple internal or external, but the API definition definitely supports it. I did find an issue that references someone setting multiple IPs of the same type as well: kubernetes/kubernetes#61921

Copy link
Member

Choose a reason for hiding this comment

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

We do not use arrays for ip address elsewhere in semantic conventions (e.g. net.peer.ip) but using arrays here here looks justified.

I would prefer to avoid the complication and just have a single ip address, but I do not see a reasonable good behavior that uses just one of the ip addresses if the source of the data can have multiple.

- id: ip
type: string
brief: >
The address of the node (dotted decimal for IPv4 or [RFC5952](https://tools.ietf.org/html/rfc5952) for IPv6.
Copy link
Member

Choose a reason for hiding this comment

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

Can you pls comment on what IP to prefer?

@SergeyKanzhelev
Copy link
Member

CC: @dashpole

Base automatically changed from master to main January 27, 2021 21:16
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 4, 2021
@bogdandrutu
Copy link
Member

Closed by (#1390)

@bogdandrutu bogdandrutu closed this Feb 4, 2021
@XSAM XSAM deleted the k8s-convention branch February 5, 2021 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include k8s.node in semantic conventions
7 participants