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

Include attribute name pluralization guidelines (#1115) #1140

Merged
merged 4 commits into from
Nov 10, 2020

Conversation

pmm-sumo
Copy link
Contributor

@pmm-sumo pmm-sumo commented Oct 23, 2020

- When attribute represents a single entity, the attribute name SHOULD BE singular.

- When attribute can represent multiple entities, the attribute name SHOULD BE pluralized
and the value type SHOULD BE an array.
Copy link
Contributor Author

@pmm-sumo pmm-sumo Oct 23, 2020

Choose a reason for hiding this comment

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

One thing I am unsure is how this impacts Erlang, which seem to not support homogenous arrays as of now. Since this case (attributes representing multiple entities) is not common, I don't think it is a concern though

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a new problem with this PR, since the specs already define an array of primitive type value for attributes, and the otel proto defines an ArrayValue for attributes.

Copy link
Member

@justinfoote justinfoote left a comment

Choose a reason for hiding this comment

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

I have a bunch of small grammatical suggestions, but overall I like it. 👍

specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
and the value type SHOULD BE an array.

- When attribute represents a measurement,
[Metric Name Pluralization Guidelines](https://github.com/pmm-sumo/opentelemetry-specification/blob/master/specification/metrics/semantic_conventions/README.md#pluralization)
Copy link
Member

Choose a reason for hiding this comment

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

❤️ (though this isn't merged yet)

@jmacd
Copy link
Contributor

jmacd commented Oct 27, 2020

In #1115 I've asked for more details about when an attribute might represent multiple entities. I've tried to focus attention on how the semantics might differ between these interpretations. I've pointed out that I think of process.command_line as a single entity whether expressed as a string or as a list of strings, and I'd like to understand the situation with potentially multiple k8s.container.name values defined.

Co-authored-by: Justin Foote <justinandrewfoote@gmail.com>
Copy link
Member

@justinfoote justinfoote left a comment

Choose a reason for hiding this comment

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

👍

@github-actions
Copy link

github-actions bot commented Nov 7, 2020

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 Nov 7, 2020
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

@pmm-sumo Thanks! It would be great if you could also add some examples like we have in #1109.

@pmm-sumo
Copy link
Contributor Author

@pmm-sumo Thanks! It would be great if you could also add some examples like we have in #1109.

Good point, just added some examples

@carlosalberto
Copy link
Contributor

@jmacd Hey, could you comment on the latest iteration?

@bogdandrutu bogdandrutu removed the Stale label Nov 10, 2020
@carlosalberto
Copy link
Contributor

@pmm-sumo Hey hey, just realized there's a small issue with a broken link, as reported by docfx:

Invalid link: 'Metric Name Pluralization Guidelines'. The file specification/metrics/semantic_conventions/README.md doesn't contain a bookmark named 'pluralization'.

Once that's fixed, we are good to merge this PR 😃

@pmm-sumo
Copy link
Contributor Author

@pmm-sumo Hey hey, just realized there's a small issue with a broken link, as reported by docfx:

Invalid link: 'Metric Name Pluralization Guidelines'. The file specification/metrics/semantic_conventions/README.md doesn't contain a bookmark named 'pluralization'.

Once that's fixed, we are good to merge this PR 😃

Yeah, I am wondering what's the best approach here. The link refers to another PR which is not merged yet. Actually, the whole whole paragraph refers to the PR. I see #1109 has 3 approvals already, so perhaps we could wait until it gets merged first? What do you think @carlosalberto ?

@carlosalberto
Copy link
Contributor

@pmm-sumo Oh, right, my bad. Let's hold it for a bit 😉

@jmacd
Copy link
Contributor

jmacd commented Nov 10, 2020

I'm trying to merge the referred-to PR but the CLA bot is wedged. I'll keep trying.

@carlosalberto carlosalberto reopened this Nov 10, 2020
@carlosalberto carlosalberto merged commit 3fee202 into open-telemetry:master Nov 10, 2020
@pmm-sumo pmm-sumo deleted the attr-name-guidance branch November 10, 2020 20:20
@pmm-sumo
Copy link
Contributor Author

Thank you @carlosalberto and @jmacd

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify pluralization guidelines for semantic conventions
7 participants