-
Notifications
You must be signed in to change notification settings - Fork 893
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 semantic conventions for Resource labels #303
Add semantic conventions for Resource labels #303
Conversation
@tigrannajaryan these LGTM as a starting point. I imagine we can add more later. I also like the formatting these are in. |
9405c94
to
46ce19d
Compare
@@ -17,3 +17,6 @@ | |||
|
|||
# Fenced code blocks should have a language specified | |||
exclude_rule 'MD040' | |||
|
|||
# Allow multiple top level headers in the same document | |||
exclude_rule 'MD025' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluded the rule to allow multiple top-level headers. I don't see a reason not to allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for that rule is probably that the h1 should be the title for the file, and a file should have only one title. After all, by putting multiple things in a file, you imply that they belong to some common group, and the h1 should be the name of that group. So I'd not change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will be an issue after splitting data-semantics file into multiple smaller ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove this change the build is going to fail (or I need to modify the headings artificially to fit the rule). I can keep the change (exclude_rule) for now and can remove it once this document is split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment than that it's temporary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this change and temporarily changed the document title to satisfy the linter. I think it is better than to modify a file that applies globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: the previous didn't work, build still fails, so I just added a TODO instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see todo =). Perhaps you need to push the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO is in data-semantic-conventions.md where I think it has a better chance to be noticed by whoever splits the document in the near future.
I addressed the comments, please have a look again. |
This document previously contained semantic conventions for Spans but was missing conventions for Resources. I copied Resource conventions from OpenCensus project [1] and also added Service conventions which were a TODO in OpenCensus conventions. [1] - https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/StandardResources.md
8e65890
to
1fcfe5b
Compare
Reviews, please have another look. I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments to clarify things... One important comment for me is the choice of UUID as a service.instanceID property.
1fcfe5b
to
230c807
Compare
All, this is ready for the final review (assuming Josh doesn't want to block it). |
@tigrannajaryan last change was made today. And it changes semantic of |
|---|---|---|---| | ||
| service.name | Logical name of the service. <br/> MUST be the same for all instances of horizontally scaled services. | `shoppingcart` | Yes | | ||
| service.namespace | A namespace for `service.name`.<br/>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. The field is optional. 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. | `Shop` | No | | ||
| service.instance.id | The string ID of the service instance. <br/>MUST be unique for each instance of the same `service.name/service.namespace` pair and SHOULD be globally unique. 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 label 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). | `627cc493-f310-47de-96bd-71410b7dec09` | Yes | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example ID is an UUID, I think it is better to suggest ids 1->N for the "ephemeral" part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting to use single-digit numbers in the example? I do not follow why UUID in the example is bad.
* Add semantic conventions for Resource labels This document previously contained semantic conventions for Spans but was missing conventions for Resources. I copied Resource conventions from OpenCensus project [1] and also added Service conventions which were a TODO in OpenCensus conventions. [1] - https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/StandardResources.md * Add namespace field and fix linting issues * Split Resource conventions into a separate document * Clarify `service.id` label and Required column meaning * Add further clarifications * Fix build * Relax service.id requirements * Add clarification that labels can be used generally to describe a resource * Remove requirement for global uniqueness of service.id
* Add semantic conventions for Resource labels This document previously contained semantic conventions for Spans but was missing conventions for Resources. I copied Resource conventions from OpenCensus project [1] and also added Service conventions which were a TODO in OpenCensus conventions. [1] - https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/StandardResources.md * Add namespace field and fix linting issues * Split Resource conventions into a separate document * Clarify `service.id` label and Required column meaning * Add further clarifications * Fix build * Relax service.id requirements * Add clarification that labels can be used generally to describe a resource * Remove requirement for global uniqueness of service.id
This document previously contained semantic conventions for Spans
but was missing conventions for Resources. I copied Resource conventions
from OpenCensus project [1] and also added Service conventions which
were a TODO in OpenCensus conventions.
[1] - https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/StandardResources.md