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

[Editorial] clarify cardinality limit #3856

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

reyang
Copy link
Member

@reyang reyang commented Jan 31, 2024

The current spec is misleading IMO "A cardinality limit is the hard limit on the number of metric streams that can be collected". It should be the limit of cardinality for a given metric.

@reyang reyang requested review from a team January 31, 2024 18:32
@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

A metric stream is defined by an attribute set:

The Metric object contains individual streams, identified by the set of Attributes. Within the individual streams, points are identified by one or two timestamps, details vary by data point type.

The cardinality limit seems to apply to these streams, not the points which are defined by timestamps.

@reyang
Copy link
Member Author

reyang commented Jan 31, 2024

A metric stream is defined by an attribute set:

The Metric object contains individual streams, identified by the set of Attributes. Within the individual streams, points are identified by one or two timestamps, details vary by data point type.

The cardinality limit seems to apply to these streams, not the points which are defined by timestamps.

This is the confusing part https://github.com/open-telemetry/opentelemetry-specification/blob/c0491fb02ad3c454d9f1c2b6b8aa51a225cf2626/specification/metrics/sdk.md#configuration-1, it seems the data model and SDK spec use "metric stream" differently:

... has an aggregation_cardinality_limit value defined for the stream ...

The proto doesn't have a concept called stream https://github.com/open-telemetry/opentelemetry-proto/blob/0a743e76ddbb34d7d46a4c3ca8f9d7bdbb81e389/opentelemetry/proto/metrics/v1/metrics.proto#L89-L92, and it has "data points" which fits the cardinality limit scenario.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

This is the confusing part https://github.com/open-telemetry/opentelemetry-specification/blob/c0491fb02ad3c454d9f1c2b6b8aa51a225cf2626/specification/metrics/sdk.md#configuration-1, it seems the data model and SDK spec use "metric stream" differently:

A view with criteria matching the instrument an aggregation is created for has an aggregation_cardinality_limit value defined for the stream, that value SHOULD be used.

The view link is in reference to:

https://github.com/open-telemetry/opentelemetry-specification/blob/c0491fb02ad3c454d9f1c2b6b8aa51a225cf2626/specification/metrics/sdk.md#view

Which talks about stream configuration. The term "stream" is defined:

Stream configuration are the parameters that define the metric stream a MeterProvider will use to define telemetry pipelines.

From this I infer that the SDK is defined in connection with the data-model definition of the term.

I'm not sure the proto definition has relevance here since it itself is mentioned to be defined in a different repository.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

I'm on board with changing the term "metric stream" if we do so comprehensively, but without that this change is adding confusion given metric stream data points already have a distinct meaning.

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Thanks @reyang, I agree this is clarifying.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 5, 2024

@reyang there seems to be precedence that our terms are defined in the data-model, not the proto: #2905 (comment)

@reyang
Copy link
Member Author

reyang commented Feb 7, 2024

I'm on board with changing the term "metric stream" if we do so comprehensively, but without that this change is adding confusion given metric stream data points already have a distinct meaning.

We discussed how to tackle this issue during the TC meeting, I've created a master issue #3866. This PR will be the first step (trying to break it down into smaller PRs).

@reyang reyang merged commit 17b3187 into open-telemetry:main Feb 15, 2024
7 checks passed
@reyang reyang deleted the reyang/clarify-cardinality-limit branch February 15, 2024 22:02
@reyang reyang mentioned this pull request Sep 27, 2024
5 tasks
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
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