open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
283 stars 175 forks source link

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

Closed dyladan closed 5 days ago

dyladan commented 1 week ago

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 ```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): > * [`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) |

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): > * [`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) > ### 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](https://img.shields.io/badge/-stable-lightgreen) |
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): > > * [`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) |

Semantic convention version

v1.28.0

Additional context

No response

dyladan commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

dyladan commented 1 week ago

@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 commented 1 week ago

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 commented 5 days ago

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