Skip to content

Require service.instance.id and define how to generate it. #3222

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

Closed
jsuereth wants to merge 1 commit into from
Closed

Require service.instance.id and define how to generate it. #3222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions semantic_conventions/resource/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ groups:
examples: ["Shop"]
- id: instance.id
type: string
requirement_level: required
Copy link
Member

@Oberon00 Oberon00 Apr 4, 2023

Choose a reason for hiding this comment

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

I don't think this should be required. I understand that generating the ID can be done in an easy way and built into the SDK, but that is not enough of an argument to make this a required attribute IMHO.

brief: >
The string ID of the service instance.
note: >
Expand All @@ -45,6 +46,19 @@ groups:
it is recommended to generate a random Version 1 or Version 4 RFC 4122 UUID
(services aiming for reproducible UUIDs may also use Version 5, see RFC 4122
for more recommendations).

SDKs are required to follow the following algorithm when generating
`service.instance.id`:
Comment on lines +50 to +51
Copy link
Member

@arminru arminru Feb 21, 2023

Choose a reason for hiding this comment

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

If we would merge any requirement like this, we will need to allow for plenty of time for our SDKs to adopt the change and gather feedback on how this works in practice before being able to call it stable.
I think we'll therefore need to carve this out of #3177 and #3202 or we'd otherwise be setting a last-minute change in stone before it's broadly implemented and verified.

Copy link
Contributor Author

@jsuereth jsuereth Feb 21, 2023

Choose a reason for hiding this comment

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

I'm going to update this PR so that the algorithmic part is carved out as its own thing that can be stabilized independently of the meaning of service.instance.id


- If the user has provided a `service.instance.id`, via environment
variable, configuration or custom resource detection, this will
always be used and honored over generated ids.
- When the SDK is running in an environment where a non-ambiguous IP
Copy link
Member

@Oberon00 Oberon00 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is impossible to detect such environments in general. Or do you have a concrete example?

More importantly, I think the IP cannot be used, because it identifies the host, not the service. Multiple different services could be running with the same IP (on the same host, or in different private networks but still reporting to the same backend). I think the id is supposed to be unique without combining it with service.name.

address exists, the ID should be set to this IP address.
- When the environment the SDK targets provides a stable identifier
matching the goals of `service.instance.id`, then this may be used.
Comment on lines +56 to +59
Copy link
Contributor

@jkwatson jkwatson Feb 17, 2023

Choose a reason for hiding this comment

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

I feel like these are pretty vague requirements. As a maintainer, I have no idea how I would go about implementing these.

Copy link
Member

@yurishkuro yurishkuro Feb 21, 2023

Choose a reason for hiding this comment

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

My problem is not that they are vague, but that they are quite arbitrary. E.g., why does IP address (2) have a priority over other possible stable native IDs (3)? Not to mention that with stacking of containers, the IP clearly does not meet the uniqueness requirement.

I think the definition at L38 already lays out the requirements for instance ID. The exact algorithm should not be required, but can be a recommendation (sans the IP part).

Copy link
Member

@jack-berg jack-berg Feb 22, 2023

Choose a reason for hiding this comment

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

I feel like these are pretty vague requirements. As a maintainer, I have no idea how I would go about implementing these.

The IP portion is vague. Figured in java we would use InetAddress.getLocalHost().getHostAddress(), with some check to determine if the IP address is ambiguous. The full proposal includes this additional text:

- 127.0.0.1/localhost is considered ambiguous.
- Multiple available IP addresses are considered ambiguous.

I do agree with @yurishkuro that "with stacking of containers, the IP clearly does not meet the uniqueness requirement".

why does IP address (2) have a priority over other possible stable native IDs (3)

What about adjusting the algorithm to allow other non-custom resource detectors to provide a service.instance.id? Currently it says "If the user has provided a service.instance.id, via environment variable, configuration or custom resource detection, this will always be used and honored over generated ids.". We could add a step to the algo to allow built-in resource detectors to provide candidates for service.instance.id. For example, container.id seems like a good fallback if the user has not defined their own.

Copy link
Member

@jack-berg jack-berg Mar 8, 2023

Choose a reason for hiding this comment

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

@jsuereth any chance you have time to make a couple small tweaks to this PR to reflect this feedback? IMO, this is really close.

Copy link
Contributor Author

@jsuereth jsuereth Apr 5, 2023

Choose a reason for hiding this comment

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

Yeah. I wanted to get the other PRs through (for which this relies on, IMO). Give me a few days to retweak thewording and add more justification on WHY we need this. There's a lot of good concerns to address, and I want to make sure the updated PR addresses all of them.

Unfortunately, can't mark this as draft due to already having one approval.

- When no other source is available the SDK MUST generate an ID. This
ID SHOULD follow version 1, 4 or 5 of RFC 4122.
examples: ["627cc493-f310-47de-96bd-71410b7dec09"]
- id: version
type: string
Expand Down
13 changes: 12 additions & 1 deletion specification/resource/semantic_conventions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ These are the attributes which MUST be provided by the SDK
as specified in the [Resource SDK specification](../sdk.md#sdk-provided-resource-attributes):

- [`service.name`](#service)
- [`service.instance.id`](#service)

## Service

Expand All @@ -67,14 +68,24 @@ as specified in the [Resource SDK specification](../sdk.md#sdk-provided-resource
|---|---|---|---|---|
| `service.name` | string | Logical name of the service. [1] | `shoppingcart` | Required |
| `service.namespace` | string | A namespace for `service.name`. [2] | `Shop` | Recommended |
| `service.instance.id` | string | The string ID of the service instance. [3] | `627cc493-f310-47de-96bd-71410b7dec09` | Recommended |
| `service.instance.id` | string | The string ID of the service instance. [3] | `627cc493-f310-47de-96bd-71410b7dec09` | Required |
| `service.version` | string | The version string of the service API or implementation. | `2.0.0` | Recommended |

**[1]:** MUST be the same for all instances of horizontally scaled services. If the value was not specified, SDKs MUST fallback to `unknown_service:` concatenated with [`process.executable.name`](process.md#process), e.g. `unknown_service:bash`. If `process.executable.name` is not available, the value MUST be set to `unknown_service`.

**[2]:** A string value having a meaning that helps to distinguish a group of services, for example the team name that owns a group of services. `service.name` is expected to be unique within the same namespace. If `service.namespace` is not specified in the Resource then `service.name` is expected to be unique for all services that have no explicit namespace defined (so the empty/unspecified namespace is simply one more valid namespace). Zero-length namespace string is assumed equal to unspecified namespace.

**[3]:** MUST be unique for each instance of the same `service.namespace,service.name` pair (in other words `service.namespace,service.name,service.instance.id` triplet MUST be globally unique). The ID helps to distinguish instances of the same service that exist at the same time (e.g. instances of a horizontally scaled service). It is preferable for the ID to be persistent and stay the same for the lifetime of the service instance, however it is acceptable that the ID is ephemeral and changes during important lifetime events for the service (e.g. service restarts). If the service has no inherent unique ID that can be used as the value of this attribute it is recommended to generate a random Version 1 or Version 4 RFC 4122 UUID (services aiming for reproducible UUIDs may also use Version 5, see RFC 4122 for more recommendations).
SDKs are required to follow the following algorithm when generating `service.instance.id`:
- If the user has provided a `service.instance.id`, via environment
variable, configuration or custom resource detection, this will
always be used and honored over generated ids.
Copy link
Contributor

@MrAlias MrAlias Mar 1, 2023

Choose a reason for hiding this comment

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

Suggested change
always be used and honored over generated ids.
always be used and honored over generated IDs.

- When the SDK is running in an environment where a non-ambiguous IP
address exists, the ID should be set to this IP address.
- When the environment the SDK targets provides a stable identifier
matching the goals of `service.instance.id`, then this may be used.
- When no other source is available the SDK MUST generate an ID. This
ID SHOULD follow version 1, 4 or 5 of RFC 4122.
Copy link
Member

@jack-berg jack-berg Feb 22, 2023

Choose a reason for hiding this comment

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

If we define this UUID stuff as part of the algorithm to generate service.instance.id, we should delete it from line 78. I.e. delete the line that reads:

If the service has no inherent unique ID that can be used as the value of this attribute it is recommended to generate a random Version 1 or Version 4 RFC 4122 UUID (services aiming for reproducible UUIDs may also use Version 5, see RFC 4122 for more recommendations).

Copy link
Contributor

@MrAlias MrAlias Mar 1, 2023

Choose a reason for hiding this comment

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

Suggested change
ID SHOULD follow version 1, 4 or 5 of RFC 4122.
ID SHOULD follow version 1, 4 or 5 of [RFC 4122](https://www.ietf.org/rfc/rfc4122.txt) UUIDs.

<!-- endsemconv -->

Note: `service.namespace` and `service.name` are not intended to be concatenated for the purpose of forming a single globally unique name for the service. For example the following 2 sets of attributes actually describe 2 different services (despite the fact that the concatenation would result in the same string):
Expand Down