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

Improve clarity of Metric Signal overview #3900

Closed

Conversation

jaydeluca
Copy link
Member

@jaydeluca jaydeluca commented Feb 24, 2024

There is no existing issue, but I am suggesting rewriting some of the Metric signal overview.

Changes

My intention was to keep the information being conveyed the same, but improve the readability.

@jaydeluca jaydeluca requested review from a team February 24, 2024 20:07
recorded using OpenTelemetry API. So user may define to aggregate those
`Measurement`s and use the context passed alongside to define additional
attributes of the resulting metric.
The primary classes for recording raw measurements are `Measure` and `Measurement`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very outdated, the current spec doesn't have a "class named Measure".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take another stab at this and try and better validate the information presented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jaydeluca! The new commits which you just pushed look much better! 👍

@jaydeluca jaydeluca marked this pull request as draft February 26, 2024 17:25
the `Metric` define their aggregation type as well as a structure of individual
measurements or Points. API defines the following types of pre-aggregated
metrics:
#### Measurements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

application that will aggregate those individual measurements into a `Metric`.
`Measure` is identified by name, description and a unit of values.
```
+-------------------+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this a bit misleading, there are two things: API/SDK components (e.g. Provider, Meter, Instrument) and data (e.g. Measurement).

This might be a better diagram? https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/metrics#programming-model

@@ -218,73 +218,83 @@ scenarios.

## Metric Signal

OpenTelemetry allows to record raw measurements or metrics with predefined
aggregation and a [set of attributes](./common/README.md#attribute).
A metric is a collection of raw measurements that can be aggregated alongside
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is going to the wrong direction. Metrics in OpenTelemetry are always preaggregated, it cannot be "a collection of raw measurements".

`Measurement` describes a single value to be collected for a `Measure`.
`Measurement` is an empty interface in API surface. This interface is defined in
SDK.
An `Instrument` describes the type of the individual values recorded by a library. It
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsloughter and others added 6 commits February 29, 2024 07:04
Fixes open-telemetry#3847 

## Changes

As discussed in the semantic conventions SIG, this PR aims to clarify
stability guarantees provided around well[-known attribute values (aka
`enums`) defined by semantic conventions:

* Changing an existing `enum` value is a breaking change.
* Adding a new value to an `enum` is not a breaking change.


##
* [x] Related issues open-telemetry#3847
* [ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) 
* [ ] Links to the prototypes (when adding or changing features)
* [x]
[`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md)
file updated for non-trivial changes
* [ ]
[`spec-compliance-matrix.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md)
updated if necessary

---------

Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
@jaydeluca
Copy link
Member Author

i messed up rebasing with intellij, closing and re-opening a new PR (#3916) to remove participants added. Sorry for the noise to those who were just added here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants