-
Notifications
You must be signed in to change notification settings - Fork 175
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
Separation of attribute definitions from attribute usages: HTTP #208
Separation of attribute definitions from attribute usages: HTTP #208
Conversation
I think this is a bit weird. Probably we'd want a specific requirement type like "needs-to-be-defind-on-usage-or-error". Otherwise I'd stay with "recommended" as default, since that is the one with the fewest requirements (opt-in requires you to either not implement it or provide some explicit configuration option if you implement it; recommended requires no implementation, but if you implement it you can do so w/o configuration option) |
Agree with that! Also see my comment here: #197 (comment) I just think we could do it in a second step. |
Then, why use opt-in in the first step instead of the default? |
I'm also fine with leaving the default ( Similar, to https://opentelemetry.io/docs/specs/semconv/url/url/, where requirement-level doesn't make any sense. |
940e2c7
to
ec69e1e
Compare
ec69e1e
to
edecc22
Compare
@open-telemetry/specs-semconv-approvers As discussed in this week's semconv WG, this is the first PR to start refactoring yaml files as described in Step1 of this proposal. Please also have a look at open-telemetry/build-tools#190, which would help with rendering the registry pages properly (without the requirement level column in the tables). |
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 think this looks pretty good!
d10e507
to
c686353
Compare
@open-telemetry/specs-semconv-approvers Can I get another round of reviews for this? Thanks! |
1ae5e24
to
35d4b7e
Compare
5b0c5d9
to
447f3ea
Compare
Added the usage of @open-telemetry/specs-semconv-approvers, @open-telemetry/specs-semconv-maintainers Are we good to proceed with 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.
It's probably too soon, but I was thinking if the registry would also benefit to follow the same directory structure as we have now in docs
. Not blocking, and we can also adapt later.
I think we have different concerns with the semantic conventions structure and the registry structure. In the semantic conventions we group / structure the content by the logical domain (and that often covers attributes from different namespaces). With the registry we provide an overview of all defined attributes. So the attributes should be easy to find by their name. That's why I think a namespace-based structuring is more beneficial for the registry. |
@open-telemetry/specs-semconv-maintainers anything blocking this? I'd really like to get this in and then move fast with the refactoring (similar to what we did with the markdown files back then). |
Conflict resolution depends on open-telemetry/build-tools#206 |
Signed-off-by: Alexander Wert <[email protected]> Update docs/attributes-registry/README.md Co-authored-by: Joao Grassi <[email protected]> Update docs/attributes-registry/README.md Co-authored-by: Joao Grassi <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
447f3ea
to
ce54b76
Compare
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.
LGTM!
This PR demonstrates how changes will look like when refactoring semantic conventions model files for the purpose of separating attribute definitions from attribute usages in concrete semantic conventions.
The changes here:
http.*
attributes are defined under /model/registry/http.yaml.Here, all attributes are defined asopt_in
.attributes-dictionary/*
that is the place to list all the available attributes in one place, grouped by the namespace (here: onlyhttp.md
so far).