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

Sort attributes by name, not by source #205

Merged
merged 8 commits into from
Sep 28, 2023

Conversation

lmolkova
Copy link
Contributor

How attributes are added to semconv is an implementation detail, rendered markdown should not depend on it.

This change sorts attributes in final md table alphabetically.

This would also reduce diffs in semconv and help us review uninteresting changes when hierarchy refactorings cause massive changes because of attributes moving in the table.

@lmolkova lmolkova requested review from a team September 18, 2023 15:30
@Oberon00
Copy link
Member

I would prefer either using a sensible order in the YAML (alphabetical might or might not be the best, there might be closely related attributes that are best put together or where there is another logical order), or one could argue that this should instead be a lint (error if input is not correctly sorted)

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 18, 2023

I would prefer either using a sensible order in the YAML (alphabetical might or might not be the best, there might be closely related attributes that are best put together or where there is another logical order), or one could argue that this should instead be a lint (error if input is not correctly sorted)

why though? what's the benefit to someone reading the semconv?

For authors, there is always a possibility of rendering individual tables per group of attributes. Having a predictable order also helps to review things.

As a reader, I want to be able to skim through the table and find my attribute in certain place, not knowing where author of semconv wanted to put it.

@Oberon00
Copy link
Member

Having a predictable order also helps to review things.

Definitely. I was under the impression that currently the order is the same as in the YAML? Do we have some random order?

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 18, 2023

Definitely. I was under the impression that currently the order is the same as in the YAML? Do we have some random order?

the order is random since:

  1. afaik nobody puts any meaning into order of attributes in yaml.
  2. even if we did, extends are implementation detail, but once we're adding one, we no longer control the relative order for defined and extended attributes.

either way, the order author had in mind is not obvious to readers, who end up with de-facto randomly ordered attributes
PTAL here (or anywhere else)

https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#common-attributes

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 18, 2023

One more observation: since we have a lot of namespaces now, most attributes are going to be referenced from somewhere and namespaces provide all the reasonable grouping.

Later on, we can consider adding sorting by requirement level and whatnot, but current random thing hurts right now during reviews.

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'm in favor of moving this direction. The simplicity gained is a big win.

@arminru arminru merged commit fb0fcaf into open-telemetry:main Sep 28, 2023
4 checks passed
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.

6 participants