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

Clarify pluralization guidelines for semantic conventions #1115

Closed
pmm-sumo opened this issue Oct 20, 2020 · 10 comments · Fixed by #1140
Closed

Clarify pluralization guidelines for semantic conventions #1115

pmm-sumo opened this issue Oct 20, 2020 · 10 comments · Fixed by #1140
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:miscellaneous For issues that don't match any other spec label

Comments

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Oct 20, 2020

What are you trying to achieve?

Most of the currently defined attributes use singular names as they typically address a single entity (e.g. os.type, db.connection.string, etc.) While it's possible to use a map or array as a value type, there are few examples found, such as process.command_line (even then it's not fully clear if arguments are always included as an array or if it's sometimes an array and sometimes a string)

There are cases (discussed in contrib#1167, where an attribute typically describes a single entity but sometimes might actually represent a collection of them.

I believe it might be helpful to clarify the rules for naming plural attributes. A similar initiative was taken for metric names recently.

Specifically, there might be two use-cases considered:

(I) Rules for attributes that are always expected to represent multiple entities

  1. Should such attribute names always use plural name?
  2. Should such value type be always an array?
  3. What about maps?

(II) Rules for attributes that typically represent a single entity but sometimes represent a collection of them

  1. Should two (singular and plural) attribute names be used (e.g. k8s.pod.container for a single instance and k8s.pod.containers for multiple instances)?
  2. Or, should a single attribute name be rather used? In such case, should it be the singular or plural form, e.g. k8s.pod.container or k8s.pod.containers?
  3. If a single attribute name is used, should it always use array type for the value or may it use string/number if only single value is present?
@pmm-sumo pmm-sumo added the spec:miscellaneous For issues that don't match any other spec label label Oct 20, 2020
@arminru arminru added the area:semantic-conventions Related to semantic conventions label Oct 20, 2020
@arminru
Copy link
Member

arminru commented Oct 20, 2020

Related: #1053

@Oberon00
Copy link
Member

While it's possible to use a map or array as a value type

Map is not possible, see #376.

@justinfoote
Copy link
Member

Related: #1109

@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 20, 2020
@andrewhsu
Copy link
Member

from the spec sig mtg today, marking as P1 to get analysis of what is needed to change so a decision can be made if the spec needs a breaking change.

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Oct 20, 2020

@bogdandrutu just wanted to confirm if I understood the suggested policy correctly; the way I understood conclusion from the SIG is following:

If the value can represent multiple entities, always use a plural name and store values in an array.

Did I get it right? In such case k8s.container.name would be renamed to k8s.pod.containers* it would be good to rename process.command_line to something else (process.command.arguments?)

* we seem to came to the conclusion it's probably not necessary in discussion under the other PR

pmm-sumo added a commit to pmm-sumo/opentelemetry-specification that referenced this issue Oct 23, 2020
pmm-sumo added a commit to pmm-sumo/opentelemetry-specification that referenced this issue Oct 23, 2020
pmm-sumo added a commit to pmm-sumo/opentelemetry-specification that referenced this issue Oct 23, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 27, 2020

Focusing on the process.command_line points above. I do not believe there is any ambiguity in allowing this attribute to be written as a single string or as an array of strings. We are trying to define semantics, and I think we all know what is meant by a "process command line" whether it is presented as a string or as an array. In both cases (string or array), it's a singular value: one string, or one array. If you are writing code to handle this annotation, I would recommend that you have a switch statement and treat both well-understood cases. I recommend this because if the natural form of a command line is a string, then that's the encoding we should use. If the natural encoding is an array of strings, then that's the encoding we should use. They mean the same thing either way.

@jmacd
Copy link
Contributor

jmacd commented Oct 27, 2020

In contrib#1167: it's not clear whether the k8s.container.name being discussed is the same concept hypothetically being described as a plural. I've asked for examples when k8s.container.name might have multiple values, but like the command_line example above, I think of attributes as single-valued whether it's one container name or one list of container names.

In #1053: here we have an inconsistency about "size" vs. "length". We have discussed this inconsistency as relating to their domains: HTTP uses "length", while messaging systems use "size". I would propose we leave these inconsistencies. These are both singular nouns, in any case.

@jmacd
Copy link
Contributor

jmacd commented Oct 27, 2020

(I) Rules for attributes that are always expected to represent multiple entities

I asked for more detail in open-telemetry/opentelemetry-collector-contrib#1167. These list-valued attributes, if used as resources, will not export into metrics systems well. Can you provide more examples? These will make sense in spans and logs more so than metrics.

(II) Rules for attributes that typically represent a single entity but sometimes represent a collection of them

In the command_line example, it's a single entity whether represented as a string or as a list of strings. In the open-telemetry/opentelemetry-collector-contrib#1167 example, there seems to be an open question about whether the attribute conceptually represents one entity or many entities. I hope we can resolve this issue by stating that attributes should always be singular and always represent a single entity. If the k8s processor example really needs to annotate multiple container names at once, I think we should dig in deeper.

@pmm-sumo May we resolve this in favor of singular names?

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Oct 27, 2020

@pmm-sumo May we resolve this in favor of singular names?

@jmacd I think that for K8s processor we should stick to singular name for container

In principle though, I am not sure if attribute names should be always singular. Looking at this PR: #1137 (which essentially fixes the command_line) we actually use a plural attribute name when an array is present

@andrewhsu andrewhsu added priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 27, 2020
@andrewhsu
Copy link
Member

from the spec sig mtg today, re-triaged this as allowed-for-ga since the change expected to address is not breaking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:miscellaneous For issues that don't match any other spec label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants