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

[rendering] Enum values not sufficiently separate from attribute footnotes #1569

Closed
dyladan opened this issue Nov 12, 2024 · 8 comments · Fixed by #1572
Closed

[rendering] Enum values not sufficiently separate from attribute footnotes #1569

dyladan opened this issue Nov 12, 2024 · 8 comments · Fixed by #1572
Labels
area:other bug Something isn't working

Comments

@dyladan
Copy link
Member

dyladan commented Nov 12, 2024

Area(s)

area:other

What happened?

Description

When markdown is rendered, the attributes table is followed by footnotes, then by tables which list values for enums. Those enum tables are not sufficiently separated from the footnotes, and they appear to be a part of the footnotes when reading. For example in HTTP client semconv the error.type table of possible values appears to be part of footnote [14] even though it is not.

Example

Below is an example taken from HTTP client semconv that shows what I'm talking about

View example markdown
**[14]:** The `url.template` MUST have low cardinality. It is not usually available on HTTP clients, but may be known by the application or specialized HTTP instrumentation.

The following attributes can be important for making sampling decisions
and SHOULD be provided **at span creation time** (if provided at all):

* [`http.request.method`](/docs/attributes-registry/http.md)
* [`server.address`](/docs/attributes-registry/server.md)
* [`server.port`](/docs/attributes-registry/server.md)
* [`url.full`](/docs/attributes-registry/url.md)

`error.type` has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

| Value  | Description | Stability |
|---|---|---|
| `_OTHER` | A fallback error value to be used when the instrumentation doesn't define a custom value. | ![Stable](https://img.shields.io/badge/-stable-lightgreen) |
Above example rendered

[14]: The url.template MUST have low cardinality. It is not usually available on HTTP clients, but may be known by the application or specialized HTTP instrumentation.
The following attributes can be important for making sampling decisions
and SHOULD be provided at span creation time (if provided at all):

error.type has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

Value Description Stability
_OTHER A fallback error value to be used when the instrumentation doesn't define a custom value. Stable

What I'm asking for is some visual separation between these elements to make it more obvious that you have moved onto a separate section like this:

View example of suggested improved rendering (heading)

[14]: The url.template MUST have low cardinality. It is not usually available on HTTP clients, but may be known by the application or specialized HTTP instrumentation.
The following attributes can be important for making sampling decisions
and SHOULD be provided at span creation time (if provided at all):

Enum Values

error.type has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

Value Description Stability
_OTHER A fallback error value to be used when the instrumentation doesn't define a custom value. Stable
View example of suggested improved rendering (horizontal line separator)

[14]: The url.template MUST have low cardinality. It is not usually available on HTTP clients, but may be known by the application or specialized HTTP instrumentation.

The following attributes can be important for making sampling decisions
and SHOULD be provided at span creation time (if provided at all):


error.type has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

| Value | Description | Stability |
< |---|---|---|
| _OTHER | A fallback error value to be used when the instrumentation doesn't define a custom value. | Stable |

Semantic convention version

v1.28.0

Additional context

No response

@dyladan dyladan added bug Something isn't working triage:needs-triage labels Nov 12, 2024
dyladan added a commit to dyladan/semantic-conventions that referenced this issue Nov 12, 2024
Add headers in order to separate enum value tables from the footnotes
above.

Fixes open-telemetry#1569
dyladan added a commit to dyladan/semantic-conventions that referenced this issue Nov 12, 2024
Add separators in order to separate enum value tables from the footnotes above.

Fixes open-telemetry#1569
dyladan added a commit to dyladan/semantic-conventions that referenced this issue Nov 12, 2024
Add separators in order to separate enum value tables from the footnotes above.

Fixes open-telemetry#1569
@dyladan
Copy link
Member Author

dyladan commented Nov 12, 2024

I created two draft PRs to better show what I'm asking for. If there in consensus on one of them i'll make it a real PR

@trask
Copy link
Member

trask commented Nov 12, 2024

Another option could be to add make the enum values into a footnote on the attribute type, e.g.

Attribute Type Description Examples Stability
http.connection.state string [1] State of the HTTP connection in the HTTP connection pool. active; idle Experimental

[1]: http.connection.state has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

Value Description Stability
active active state. Experimental
idle idle state. Experimental

@dyladan
Copy link
Member Author

dyladan commented Nov 12, 2024

Another option could be to add make the enum values into a footnote on the attribute type, e.g.

Attribute Type Description Examples Stability
http.connection.state string [1] State of the HTTP connection in the HTTP connection pool. active; idle Experimental
[1]: http.connection.state has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

Value Description Stability
active active state. Experimental
idle idle state. Experimental

This solves another issue which is that the attribute table doesn't say anything about a list of acceptable values. If you look at the semconv I linked originally, nowhere does it say in the attribute table that error.type, http.request.method, or any other enum, is an enum that has specific allowable values. If you don't happen to scroll down you might miss that entirely.

edit: as long as we're considering larger changes, i'd also suggest to call it "extensible enum" instead of "string" for type.

@joaopgrassi
Copy link
Member

I like the first idea of having a heading for the enums. With that, we could also add subheadings for each enum like

Enums

http.connection.state

table...

And then we could add an anchor on the attribute table that goes to that section.

@jsuereth
Copy link
Contributor

@joaopgrassi What would you want to do about markdown-toc in that case?

@dyladan
Copy link
Member Author

dyladan commented Nov 15, 2024

@joaopgrassi What would you want to do about markdown-toc in that case?

What's the issue with markdown-toc? Should work the same way right?

@joaopgrassi
Copy link
Member

I guess what @jsuereth means is that they will show up as part of the TOC which might be a lot. We should be able to remove them from TOC I guess by some obscure code comment

@dyladan
Copy link
Member Author

dyladan commented Nov 21, 2024

I decided to go with the separators. It's just easier and avoids toc issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:other bug Something isn't working
Projects
None yet
4 participants