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

Define a common algorithm for service.instance.id #312

Merged
merged 31 commits into from
Feb 23, 2024

Conversation

jpkrohling
Copy link
Member

@jpkrohling jpkrohling commented Sep 12, 2023

Related to open-telemetry/opentelemetry-specification#311, built on top of open-telemetry/opentelemetry-specification#3222 by @jsuereth.

Changes

Enhances the existing service.instance.id convention with a common algorithm for SDKs to use when generating that value.

Merge requirement checklist

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling
Copy link
Member Author

Linters be linters: one complains that the readme.md needs to be reformatted, the other complains about a lack of empty line (which exists in the source of the information). Is there a way to ignore a linter, or to force the new line to be added to the output md file?

docs/resource/README.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
model/resource/service_experimental.yaml Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member Author

I believe this is ready for a wider review. I'll fix the changelog conflicts prior to merging.

@jpkrohling jpkrohling marked this pull request as ready for review September 13, 2023 08:28
@jpkrohling jpkrohling requested review from a team September 13, 2023 08:28
@AlexanderWert
Copy link
Member

Thinking of ECS alignment / merger in this context.

In ECS there is the service.node.* namespace with the fields service.node.name and service.node.roles.

The service.node.name is basically the same as the service.instance.id attribute discussed here.
Is there a chance that we align on ECS' service.node.name while keeping the algorithm / description proposed in this PR?

I can also create a separate issue for that, if it's preferable.

docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member Author

I can also create a separate issue for that, if it's preferable.

I would prefer that, if possible.

@svrnm
Copy link
Member

svrnm commented Sep 25, 2023

@jpkrohling you might be interested in this: containerd/containerd#8185, I can add some more context if this helps for solving your questions to tell 2 containers apart within a container.

@jpkrohling
Copy link
Member Author

Status: I got stuck on the container ID for some time and despite stateful and daemon sets being common, I'll treat them as cases that need to be handled manually by users or tooling (like the operator), recommending them to set the container name via semantic conventions for that.

docs/resource/README.md Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member Author

Status: I believe this is ready to go. I spent more time than I wanted on trying to find a good solution for stateful and daemon sets for Kubernetes, but I couldn't. I think the current spec is good enough for most of the cases out there, and with perhaps a change or two to the operator, we should have good coverage on the Kubernetes world as well.

@jpkrohling
Copy link
Member Author

@jsuereth , @tigrannajaryan , @gouthamve , @pyohannes , can you please review this again? I'd like to wrap this up.

docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
docs/resource/README.md Outdated Show resolved Hide resolved
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Implementations, such as SDKs, are recommended to generate a random Version 1 or Version 4 [RFC
4122](https://www.ietf.org/rfc/rfc4122.txt) UUID, but are free to use an inherent unique ID as the source of
this value if stability is desirable. In that case, the ID SHOULD be used as source of a UUID Version 5 and
SHOULD use the following UUID as the namespace: `4d63009a-8d0f-11ee-aad7-4c796ed8e320`.
Copy link
Member

Choose a reason for hiding this comment

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

don't understand what it means to "use UUID as the namespace"

Copy link
Member Author

Choose a reason for hiding this comment

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

UUID v5 has a namespace. Previous versions of the PR had an example showing this, but perhaps this would help see where this namespace is used: https://pkg.go.dev/github.com/google/uuid#NewSHA1

Copy link
Member

Choose a reason for hiding this comment

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

The RFC also explains how namespace is used to generate a v5 UUID: https://datatracker.ietf.org/doc/html/rfc4122#page-13

maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…e detection

`service.instance.id` is a recommended value. This commit adds it to the alibaba resource detector.
It uses the `serial-number` value from the instance metadata, which has the description:
```
The serial number of the instance.
Example: 4acd2b47-b328-4762-852f-998****
```
Which alignes with the definition for `service.instance.id` from [here](open-telemetry/semantic-conventions#312)

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…e detection

`service.instance.id` is a recommended value. This commit adds it to the alibaba resource detector.
It uses the `serial-number` value from the instance metadata, which has the description:
```
The serial number of the instance.
Example: 4acd2b47-b328-4762-852f-998****
```
Which alignes with the definition for `service.instance.id` from [here](open-telemetry/semantic-conventions#312)

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…ction

`service.instance.id` is a recommended value.
Implements `service.instance.id` from [here](open-telemetry/semantic-conventions#312). Uses `randomUUID` as is similar implemented
on other languages, such as the Java SDK.

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…e detection

`service.instance.id` is a recommended value. This commit adds it to the alibaba resource detector.
It uses the `serial-number` value from the instance metadata, which has the description:
```
The serial number of the instance.
Example: 4acd2b47-b328-4762-852f-998****
```
Which alignes with the definition for `service.instance.id` from [here](open-telemetry/semantic-conventions#312)

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…e detection

`service.instance.id` is a recommended value. This commit adds it to the alibaba resource detector.
It uses the `serial-number` value from the instance metadata, which has the description:
```
The serial number of the instance.
Example: 4acd2b47-b328-4762-852f-998****
```
Which alignes with the definition for `service.instance.id` from [here](open-telemetry/semantic-conventions#312)

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
maryliag added a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 3, 2024
…ction

`service.instance.id` is a recommended value.
Implements `service.instance.id` from [here](open-telemetry/semantic-conventions#312). Uses `randomUUID` as is similar implemented
on other languages, such as the Java SDK.

Part Of open-telemetry#2065

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.