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

[chore] System Semantic Conventions Non-Normative Guidance #1618

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

braydonk
Copy link
Contributor

Changes

This PR adds non-normative guidance from the System Semantic Conventions Working Group. This is added in a new groups folder in non-normative, and a system subfolder in groups. The docs written here were already discussed in a Google doc where we were originally collaborating on this, a link to which can be shared directly if needed.

Merge requirement checklist

@braydonk braydonk requested review from a team as code owners November 26, 2024 15:01
@braydonk braydonk requested a review from a team November 26, 2024 15:01
@braydonk braydonk changed the title System Semantic Conventions Non-Normative Guidance [chore] System Semantic Conventions Non-Normative Guidance Nov 26, 2024
@braydonk braydonk added Skip Changelog Label to skip the changelog check area:system labels Nov 26, 2024
@mx-psi mx-psi self-requested a review November 26, 2024 15:56
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

I really like this doc!

I don't think we have similar precedents of "why we designed it in this way" documented (the closest analogy is OTEP), but I wish we had more of these.
We might find a better place for it within the repo over time if we'll have more docs like this.

docs/non-normative/groups/system/design-philosophy.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/design-philosophy.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/design-philosophy.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/design-philosophy.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/design-philosophy.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
@braydonk braydonk force-pushed the system_semconv_non_normative branch from e980f13 to e051e87 Compare November 27, 2024 14:30
@braydonk
Copy link
Contributor Author

Did a first pass of easy comments to address, will make some time soon to go through the comments that require more thought!

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM with a question/suggestion.

* General disk and network metrics
* Universal system/process information (names, identifiers, basic specs)

Some Specialist Class examples:
Copy link
Member

Choose a reason for hiding this comment

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

While the whole description of the rationale here is exactly how it should be, I think we miss the part of having a set of rules/guidelines/sanity-checks that would help somebody in the future to decide into which directory a metric or attribute fall into. This might not be quite easy to define because of the nature of this problem but maybe it would worth adding a section in the bottom suggesting how this kind of situations should be handled in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do have a case study below for process.linux.cgroup; perhaps I can adapt this to more general rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 487af83

docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
docs/non-normative/groups/system/use-cases.md Outdated Show resolved Hide resolved
* Machine name
* ID (relevant to its context, could be a cloud provider ID or just base machine ID)
* OS information (platform, version, architecture, etc)
* Number of CPU cores
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be "CPU information" instead? We have a bunch of those here

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Approving, I left a few non-blocking comments above :)

@mx-psi
Copy link
Member

mx-psi commented Nov 29, 2024

I marked #1403 and #1578 to be closed by this PR, please let me know if this is not right

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

I love writing this down.

The categorization of "Two Class Design Strategy" I think we should move to general non-normative guidance for all semantic conventions to follow.

@mx-psi
Copy link
Member

mx-psi commented Dec 19, 2024

What is missing for this to be merged?

@braydonk
Copy link
Contributor Author

I'm finishing up edits for the remaining open comments, will be pushing this morning.

This PR adds non-normative guidance from the System Semantic Conventions
Working Group. This is added in a new `groups` folder in
`non-normative`, and a `system` subfolder in `groups`. The docs written
here were already discussed in a Google doc where we were originally
collaborating on this, a link to which can be shared directly if
needed.
@braydonk braydonk force-pushed the system_semconv_non_normative branch from e051e87 to 01f43e9 Compare December 19, 2024 18:25
@braydonk
Copy link
Contributor Author

I've pushed up two new commits:

487af83: Addresses review comments. I will re-request review from those who still had open comments.

01f43e9: To address the issue with the markdown files having really long lines, I have set up Prettier to apply to these markdown files and wrap them at 80 characters. Did this in a separate commit so it wasn't too difficult to see exactly how I addressed open comments.

@lmolkova
Copy link
Contributor

PTAL at the related #1707 - it's my attempt to document overall semconv guidance (only attribute definition so far). There are some intersections.

Copy link

github-actions bot commented Jan 9, 2025

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 Jan 9, 2025
@lmolkova lmolkova removed the Stale label Jan 9, 2025
@lmolkova
Copy link
Contributor

lmolkova commented Jan 9, 2025

@braydonk it'd be awesome to get it merged. Is there anything controversial that we can't follow up on later?

@braydonk
Copy link
Contributor Author

braydonk commented Jan 9, 2025

@lmolkova if the latest conversation is resolved, then I don't think there's anything controversial left here. I think much of that system prefix naming discussion has moved to #1711 and related PRs, so if any of that marks a meaningful shift in guidance I can follow up and update the guidance. Other than that, I'm all set for merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:system Skip Changelog Label to skip the changelog check
Projects
Status: Needs More Approval
9 participants