-
Notifications
You must be signed in to change notification settings - Fork 106
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
Support explicit_bucket_boundaries advisory parameters #628
Conversation
Codecov ReportAll modified lines are covered by tests ✅ 📢 Thoughts on this report? Let us know!. |
840b120
to
d43a1f5
Compare
Fixes #629 |
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:
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. |
Thanks, I will look at it |
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had one suggestion.
@tsloughter ready |
ExplicitBucketBoundaries
is almost stable, already approved, only needs to be mergedSee open-telemetry/opentelemetry-specification#3694
The added test should be explicative about the behaviour