Give guideline of not using unit names in attributes (#1053) #1307
Conversation
alexvanboxel
force-pushed
the
feature/names
branch
3 times, most recently
from
0ee276a
to
0493c62
Compare
| - Names should not contain unit names, and their values should be measured in the | ||
| smallest practical unit size. For example, `http.request_content_length` does not | ||
| have bytes in its name and uses the smallest unit size: byte. If the smallest | ||
| unit is a fraction, the prefix is not part of the name. |
There was a problem hiding this comment.
I think you need an example here. Would this mean that we use execution_time instead of execution_time_nanos?.
There was a problem hiding this comment.
I've been going over all the attributes defined in the spec and none mention the unit fraction. The only attribute that uses the unit name is message_payload_size_bytes. So yes, I think having execution_time would be more consistent with the rest of the attributes.
| @@ -49,6 +49,11 @@ Names SHOULD follow these rules: | |||
| name prohibits existence of an equally named namespace in the future, and vice | |||
| versa: any existing namespace prohibits existence of an equally named | |||
| attribute or label key in the future. | |||
|
|
|||
| - Names should not contain unit names, and their values should be measured in the | |||
There was a problem hiding this comment.
What is the justification for that vs they should always contain unit names?
There was a problem hiding this comment.
Having the unit names in the attribute names would in most cases redundant. Example execution_time would be silly to have seconds in the name. The same for message_payload_size_bytes, the fact that it's bytes is redundant information.
We could discuss the fraction names (nanos, MiB, ...) separately: here it's about the fact that it's not used in any of the attributes yet.
There was a problem hiding this comment.
This change is reasonable for metric names which support units natively. Span or log attributes have no such support. I don't think this is a good blanket recommendation for all cases. There may be cases when units are ambiguous. In the example given execution_time it is not obvious to me that the execution time should be in seconds. Why would I assume that? Why not milliseconds?
There was a problem hiding this comment.
I think it would make sense to require all spec semantic conventions to always use nanoseconds for any timespans. That way it would be unambiguous.
There was a problem hiding this comment.
I reworded the wording not to talk about the smallest practical unit, just to focus on the naming. Talking about this could be interesting, but I propose that someone takes this out of the scope of this PR.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
alexvanboxel
force-pushed
the
feature/names
branch
from
0493c62
to
9159750
Compare
…1053) Add a paragraph in the attribute name guidelines that discourage the use of unit names and prefixes. Add note about historical naming violation in messaging.md, the attribute names message_payload_size_bytes and message_payload_compressed_size_bytes about the incorrect use of bytes in the name.
alexvanboxel
force-pushed
the
feature/names
branch
from
9159750
to
cf1c360
Compare
There was a problem hiding this comment.
@alexvanboxel sorry, I am not convinced that this change is an improvement for Spans. For metrics, sure, I completely agree, it is wrong to have units in the names. For span attributes, I disagree.
I would be fine to merge a change like this if it applied only to metric names. As-is it applies to spans too so I am blocking it until we figure this out (I am willing to be convinced).
are you opposed to units (ex. bytes) or fractions (ex. nano, milli, ...) or both? |
I am opposed to the blanket requirement which says we cannot have units in attribute names. I think there are valid cases when it is useful to have units in span attribute names, because not having the unit in the name makes the value ambiguous. For example |
| @@ -181,6 +181,8 @@ For batch receiving and processing (see the [Batch receiving](#batch-receiving) | |||
| Even though in that case one might think that the processing span's kind should be `INTERNAL`, that kind MUST NOT be used. | |||
| Instead span kind should be set to either `CONSUMER` or `SERVER` according to the rules defined above. | |||
|
|
|||
| **Note:** The payload size names (`message_payload_compressed_size_bytes` and `message_payload_size_bytes`) don't comply with the [Attribute and Label Naming](../../common/attribute-and-label-naming.md), it should not contain `bytes`. This is due to historical reasons. | |||
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
I'm fine with dropping this PR if no consensus can be reached. I like to have consistency, and this PR only resulted from the issue I created. Do we drop this PR? |
|
@alexvanboxel For metrics it looks right IMHO, so worth considering going metrics-only from here. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.
Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
Fixes #1053
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes. IfCHANGELOG.mdis updated,also be sure to update
spec-compliance-matrix.mdif necessary.Related issues #
Related oteps #