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

Clarification around the relationship between metric views and metric streams #3829

Closed
danielgblanco opened this issue Jan 16, 2024 · 2 comments · Fixed by #3842
Closed

Clarification around the relationship between metric views and metric streams #3829

danielgblanco opened this issue Jan 16, 2024 · 2 comments · Fixed by #3842
Assignees
Labels
[label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:metrics Related to the specification/metrics directory

Comments

@danielgblanco
Copy link
Contributor

What are you trying to achieve?

Opening this as a follow up on #2356 which was closed a while ago. I think the measurement processing part of the spec has not been changed since then so this still applies.

To folks not familiar with all parts of the spec or existing implementations, I believe the current measurement processing logic as explained in the spec does not make it clear that multiple streams will be generated if multiple views are matching the same instrument, potentially generating conflicts. As @jack-berg explained in the original issue, this is more confusing when the stream configuration of two or more metric views set different stream properties (e.g. one setting aggregation and the other attribute keys), which could lead end-users to incorrectly expect these to be "merged". The action of creating different streams per view seems to be implied by the following sentence:

Try to apply the View’s stream configuration.

I think this could be phrased better to explain that each view will create one or many streams, and those may result in conflicting metric identities. Furthermore, the examples in the section below don't seem to align with the spec. For instance, this one:

# Counter X will be exported as delta sum
# Histogram Y and Gauge Z will be exported with 2 attributes (a and b)
meter_provider
    .add_view("X", aggregation=SumAggregation())
    .add_view("*", attribute_keys=["a", "b"])
    .add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()),
              temporality=lambda kind: Delta if kind in [Counter, AsyncCounter, Histogram] else Cumulative)

Seems to imply that these views will generate 3 streams:

  • Counter X with default attributes and Sum aggregation
  • Histogram Y with attribute keys a and b and default aggregation
  • Gauge Z with attribute keys a and b and default aggregation

In reality, these views would generate a conflict on X, as it generates two different conflicting streams with the same name. In the case of Java, currently, X would result in the relevant warning message and two metric streams:

  • Counter X with default attributes and Sum aggregation.
  • Counter X with a and b attributes and default aggregation.

Making it clear that views generate different streams, and how conflicts are resolved at that level, could also help explain the special behaviour of the drop aggregation, which can be combined with other views matching the same instrument safely, as it cannot result in conflicts. This could be covered by an example.

What did you expect to see?

  1. Clearer guidance around measurement processing (no changes to logic).
  2. Current examples corrected to align with guidance.
  3. More examples to cover cases of drop aggregation combined with others.

I can work on a PR to add these if agreed.

@danielgblanco danielgblanco added the spec:metrics Related to the specification/metrics directory label Jan 16, 2024
@reyang
Copy link
Member

reyang commented Jan 24, 2024

I'll send a fix on the examples.

@reyang reyang self-assigned this Jan 24, 2024
@arminru arminru added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Jan 24, 2024
@danielgblanco
Copy link
Contributor Author

@reyang I just opened that PR, sorry for the delay

@reyang reyang assigned danielgblanco and unassigned reyang Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants