Skip to content

[sdk-metrics] Promote overflow attribute from experimental to stable #5909

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.

Sign up for GitHub

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

Merged
CodeBlanch merged 14 commits into from
Oct 25, 2024
Merged

[sdk-metrics] Promote overflow attribute from experimental to stable #5909

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7070393
Promote overflow attribute from experimental to stable
xiang17 Oct 18, 2024
a349acf
update wording in markdowns
xiang17 Oct 18, 2024
737c3c9
update readme regarding the warning the first time an overflow is det…
xiang17 Oct 18, 2024
9e4fe9b
Remove MetricPointCaptHitMessage
xiang17 Oct 23, 2024
bc41d1a
Update comments related to overflow attribute being optional
xiang17 Oct 23, 2024
127025f
Update README and CHANGELOG
xiang17 Oct 23, 2024
f3d1257
The measurement won't be dropped. It will be aggregated with the over…
xiang17 Oct 23, 2024
0823e2d
Update src/OpenTelemetry/CHANGELOG.md
xiang17 Oct 23, 2024
aeceb18
reword
xiang17 Oct 23, 2024
ba83250
Merge branch 'main' into xiang17/OverflowAttribute
xiang17 Oct 23, 2024
b72090e
typo
xiang17 Oct 23, 2024
6c67e3f
improve README and CHANGELOG
xiang17 Oct 24, 2024
a118842
update wording
xiang17 Oct 24, 2024
e4e21ca
Merge branch 'main' into xiang17/OverflowAttribute
CodeBlanch Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update README and CHANGELOG
  • Loading branch information
@xiang17
xiang17 committed Oct 23, 2024
commit 127025f46c868ce11a0399a9bc62f4512ccd92ea
21 changes: 4 additions & 17 deletions docs/metrics/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,23 +388,10 @@ and the `MetricStreamConfiguration.CardinalityLimit` setting. Refer to this
[doc](../../docs/metrics/customizing-the-sdk/README.md#changing-the-cardinality-limit-for-a-metric)
for more information.

Given a metric, once the cardinality limit is reached, any new measurement which
cannot be independently aggregated because of the limit will be dropped or
aggregated using the [overflow
attribute](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute)
(if enabled). A warning is written to the [self-diagnostic
log](../../src/OpenTelemetry/README.md#self-diagnostics) the first time an
overflow is detected for a given metric.

> [!NOTE]
> Overflow attribute was introduced in OpenTelemetry .NET
[1.6.0-rc.1](../../src/OpenTelemetry/CHANGELOG.md#160-rc1). It was an
experimental feature which can be turned on by setting the environment
variable `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE=true`. After
the [OpenTelemetry
Specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute)
become stable, the environment variable is removed and this feature is enabled
by default.
Starting with version 1.10.0, given a metric, once the cardinality limit is
Copy link
Member

@cijothomas cijothomas Oct 23, 2024

Choose a reason for hiding this comment

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

i think its best to mention the pre 1.10.0 behavior too here.
Versions prior to 1.10.0 had a different behavior when cardinality limit was hit. The measurements was either dropped (default) and an internal log was emitted once or aggregated using overflow attribute(opt-in basis)

(we have some places in this repo where were use such style in document)

Copy link
Member

@rajkumar-rangaraj rajkumar-rangaraj Oct 23, 2024

Choose a reason for hiding this comment

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

I feel it's better if the README reflects the current scenario. Adding prior behavior may increase the length of the document.

Copy link
Member

@CodeBlanch CodeBlanch Oct 23, 2024

Choose a reason for hiding this comment

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

I think the whole thing is a bit confusing 🤣

Here is my stab at improving it:

Given a metric, once the cardinality limit is reached, any new measurement which cannot be independently aggregated because of the limit will be dropped. Starting with version 1.10.0, when a measurement is dropped, the the overflow attribute is updated automatically.

I think what is important is the first statement "Given a metric, once the cardinality limit is reached, any new measurement which cannot be independently aggregated because of the limit will be dropped." That is a hard stop thing. The way it is currently written, it seems this is new to 1.10.0 😄

Copy link
Contributor Author

@xiang17 xiang17 Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel this is tricky. One is the Previous behavior where there is default and experimental opt-in overflow attribute. The other one is the New behavior with the always on overflow attribute. It will also be too long if we explain everything in README.

I've changed the README to include minimal info to mention things has changed a few times, and put more details in the CHANGELOG in case anyone is really interested and want to better understand.

Copy link
Contributor Author

@xiang17 xiang17 Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think what is important is the first statement "Given a metric, once the cardinality limit is reached, any new measurement which cannot be independently aggregated because of the limit will be dropped." That is a hard stop thing. The way it is currently written, it seems this is new to 1.10.0 😄

That's a good point. I've updated it to make that first statement more prominent and followed with how our SDK handles and how approaches changed.

One small thing, the spec seems to not see it as a "drop" when it's done with the overflow attribute, but rather a "synthetic aggregation". So it's still "aggregated", but not "independently aggregated".

An overflow attribute set is defined, containing a single attribute otel.metric.overflow having (boolean) value true, which is used to report a synthetic aggregation of the Measurements that could not be independently aggregated because of the limit.

reached, any new measurement which cannot be independently aggregated because of
the limit will be dropped or aggregated using the [overflow
attribute](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute).

When [Delta Aggregation
Temporality](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#temporality)
Expand Down
8 changes: 5 additions & 3 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ Notes](../../RELEASENOTES.md).

## Unreleased

* The opt-in overflow attribute feature which can be enabled by setting the
environment variable `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE`
to `true` is now enabled by default and supported in stable builds.
* Removed the `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE`
environment variable since the feature has moved from experimental to stable.
The overflow attribute feature is now enabled by default.
No internal logs will be emitted when the limit is hit, as the
`otel.metric.overflow` attribute indicates when a cardinality cap has occurred..
([#5909](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5909))

## 1.10.0-beta.1
Expand Down