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

Support explicit_bucket_boundaries advisory parameters #628

Merged
merged 7 commits into from
Oct 2, 2023

Conversation

albertored
Copy link
Contributor

@albertored albertored commented Sep 19, 2023

ExplicitBucketBoundaries is almost stable, already approved, only needs to be merged

See open-telemetry/opentelemetry-specification#3694

The added test should be explicative about the behaviour

@albertored albertored requested a review from a team September 19, 2023 14:44
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

All modified lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

@albertored
Copy link
Contributor Author

Fixes #629

@RoadRunnr
Copy link
Contributor

managed to crash it.

Give this sequence:

    Meter = opentelemetry_experimental:get_meter(),
    true = otel_meter_server:add_view(
             #{instrument_kind => ?KIND_HISTOGRAM},
             #{aggregation_module => otel_aggregation_histogram_explicit,
               aggregation_options => #{boundaries => [10, 100]}}),
    Histogram = otel_histogram:create(Meter, histogram, #{}),
    true = otel_meter_server:add_view(
             #{instrument_name => histogram},
             #{
               aggregation_module => otel_aggregation_histogram_explicit,
               aggregation_options => #{boundaries => [10, 20, 30, 40, 50, 60, 70, 80, 100]}}),
    ok = otel_histogram:record(Histogram, 2000, #{<<"a">> => <<"1">>})

It will crash with:

{badarg,
    [{counters,add,
         [{write_concurrency,#Ref<0.388515526.2174353409.18119>},10,1],
         [{error_info,#{module => erl_erts_errors}}]},
     {otel_aggregation_histogram_explicit,aggregate,4,
         [{file,
              "/usr/src/erlang/opentelemetry-erlang/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl"},
          {line,175}]},
     {otel_aggregation,maybe_init_aggregate,4,
         [{file,
              "/usr/src/erlang/opentelemetry-erlang/apps/opentelemetry_experimental/src/otel_aggregation.erl"},
          {line,49}]},

Root problem is that the the second view does not update the counter bucket in the histogram view.

There is nothing in the OpenTelemetry specification that indicates that doing something like that is not permitted.
Even the code suggest that it should be fine to update views like that. Of course, I would expect that such an updated flushes any already collected data points.

@albertored
Copy link
Contributor Author

Thanks, I will look at it

@tsloughter
Copy link
Member

We likely don't want to support updating views -- I don't think any implementation does. But also don't want to crash :)

There has been talk about this in the metrics SIG, about whether you can even add views after the meter provider has started or not.

@albertored
Copy link
Contributor Author

albertored commented Sep 25, 2023

We do not handle very well duplicated metrics, see https://opentelemetry.io/docs/specs/otel/metrics/sdk/#measurement-processing. In the case of the example you provided we are defining a duplicated metric with different boundaries, it is not exactly what is written in the spec but I think we should drop the second view

Copy link
Member

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

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

Had one suggestion.

@albertored
Copy link
Contributor Author

@tsloughter ready

@tsloughter tsloughter merged commit 623a1fe into open-telemetry:main Oct 2, 2023
21 checks passed
@albertored albertored deleted the advisory-parameters branch October 2, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants