Metrics terminology

[SergeyKanzhelev]

Fix #45

[songy23]

LGTM

[iredelmeier]

Core content LGTM, however:

• I believe it's worth pulling out separate sections (i.e., with headers) for the definitions of Measure and Measurement
• Some of the content feels like it's more about why and how than what, where I believe that the terminology sections should be primarily about what. In other words, someone implementing the spec shouldn't need to reference the terminology sections in order to understand how to implement it; other files should be sufficient. Given that those files should be sufficient, having the why and how be in both places makes it more likely that the content will eventually diverge and become stale or otherwise inaccurate.
  • e.g., Because of this, Metrics puts minimal constraints on the data (e.g. which characters are allowed in keys), and code dealing with Metrics should avoid validation and sanitization of the Metrics data. Instead, pass the data to the backend, rely on the backend to perform validation, and pass back any errors from the backend.
• DistributedContext is already defined elsewhere. We should link instead of re-defining it as, again, duplication leads to inaccuracy.
• Some grammar and typo fixes. I started going through these (and don't mind doing so!), but it probably makes sense to hold off until after other things are addressed?

General comment: having hard wraps in Markdown files makes editing, changing, and tracking history a lot harder than having only natural breaks at paragraph ends with soft wraps in editors :/ (Most Markdown viewers - GitHub's included - are able to handle this decently.)

[SergeyKanzhelev]

• I believe it's worth pulling out separate sections (i.e., with headers) for the definitions of Measure and Measurement

This separation will be done in specification documents. This is terminology and it's hard to separate definition of a Measure from Measurement.

• Some of the content feels like it's more about why and how than what, where I believe that the terminology sections should be primarily about what. In other words, someone implementing the spec shouldn't need to reference the terminology sections in order to understand how to implement it; other files should be sufficient. Given that those files should be sufficient, having the why and how be in both places makes it more likely that the content will eventually diverge and become stale or otherwise inaccurate.

I have the same feeling. Was planning to move some of these into SDK section when it will be created.

• DistributedContext is already defined elsewhere. We should link instead of re-defining it as, again, duplication leads to inaccuracy.

There is also SpanContext that can be passed. I was thinking I will leave specifics to the API and only mention vaguely "context".

• Some grammar and typo fixes. I started going through these (and don't mind doing so!), but it probably makes sense to hold off until after other things are addressed?

It's time!

General comment: having hard wraps in Markdown files makes editing, changing, and tracking history a lot harder than having only natural breaks at paragraph ends with soft wraps in editors :/ (Most Markdown viewers - GitHub's included - are able to handle this decently.)

Worth a separate discussion. Please file an issue if you feel strongly. It's a tabs-spaces level conversation. The idea way to resolve it is with the linter. @yurishkuro was suggesting some that they are using in Jaeger.

[SergeyKanzhelev]

@iredelmeier for the grammar changes - would it work to merge it first and then work on grammar separately? Or you'd prefer to do it as part of this PR?

[iredelmeier]

• I believe it's worth pulling out separate sections (i.e., with headers) for the definitions of Measure and Measurement

This separation will be done in specification documents. This is terminology and it's hard to separate definition of a Measure from Measurement.

Clarifying: I meant separate sections within the same doc. Very much agree that it's hard to separate the definitions.

• Some of the content feels like it's more about why and how than what, where I believe that the terminology sections should be primarily about what. In other words, someone implementing the spec shouldn't need to reference the terminology sections in order to understand how to implement it; other files should be sufficient. Given that those files should be sufficient, having the why and how be in both places makes it more likely that the content will eventually diverge and become stale or otherwise inaccurate.

I have the same feeling. Was planning to move some of these into SDK section when it will be created.

• DistributedContext is already defined elsewhere. We should link instead of re-defining it as, again, duplication leads to inaccuracy.

There is also SpanContext that can be passed. I was thinking I will leave specifics to the API and only mention vaguely "context".

Not sure that I follow your meaning?

• Some grammar and typo fixes. I started going through these (and don't mind doing so!), but it probably makes sense to hold off until after other things are addressed?

It's time!

Per your added comment, I think a separate PR makes the most sense to keep the core content unblocked :)

General comment: having hard wraps in Markdown files makes editing, changing, and tracking history a lot harder than having only natural breaks at paragraph ends with soft wraps in editors :/ (Most Markdown viewers - GitHub's included - are able to handle this decently.)

Worth a separate discussion. Please file an issue if you feel strongly. It's a tabs-spaces level conversation. The idea way to resolve it is with the linter. @yurishkuro was suggesting some that they are using in Jaeger.

👍

[SergeyKanzhelev]

@iredelmeier would you than sign off so I can merge? Or anything needed to be addressed before the merge?

[jmacd]

Another way of stating this is that "predefined aggregation" should be independent of "predefined labels". I think if predefined labels were available for all measurements, the concept of "Raw" measurement wouldn't be needed: raw would just mean measurements without pre-defined aggregation.

[SergeyKanzhelev]

it's a good point. label keys are predefined, not the values. I'll rephrase

[jmacd]

👍

[SergeyKanzhelev]

PR was active for more than a day. Merging