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.

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

fix: Coerce aggregation_temporality value to a symbol #1741

Merged
merged 3 commits into from
Oct 16, 2024
Merged

fix: Coerce aggregation_temporality value to a symbol #1741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d9fb2a
fix: Coerce aggregation_temporality to symbol
kaylareopelle Oct 7, 2024
ed080e5
Merge branch 'main' into temporality-preference-fix
kaylareopelle Oct 7, 2024
5d961a2
Merge branch 'main' into temporality-preference-fix
kaylareopelle Oct 15, 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
3 changes: 2 additions & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ def initialize(
boundaries: DEFAULT_BOUNDARIES,
record_min_max: true
)
@aggregation_temporality = aggregation_temporality

@aggregation_temporality = aggregation_temporality.to_sym
@boundaries = boundaries && !boundaries.empty? ? boundaries.sort : nil
@record_min_max = record_min_max
end
Expand Down
2 changes: 1 addition & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class Sum

def initialize(aggregation_temporality: ENV.fetch('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE', :delta))
# TODO: the default should be :cumulative, see issue #1555
@aggregation_temporality = aggregation_temporality
@aggregation_temporality = aggregation_temporality.to_sym
Copy link
Contributor

@arielvalentin arielvalentin Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Non-Blocking Comment:

Can this be user specified or is there a constraint on the temporality preference?

I.e. should this be open ended or should it be a sort of enum object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! I think an enum would be a good addition here.

According to the spec, the only valid values here are :delta or :cumulative. The OTLP exporter will fall back to unspecified otherwise.

We don't have support for :cumulative aggregation yet, so really, the only true option should just be :delta.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue to fix this as a follow-up: #1742

end

def collect(start_time, end_time, data_points)
Expand Down
21 changes: 21 additions & 0 deletions metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,27 @@
let(:start_time) { (Time.now.to_r * 1_000_000_000).to_i }
let(:end_time) { ((Time.now + 60).to_r * 1_000_000_000).to_i }

describe '#initialize' do
it 'defaults to the delta aggregation temporality' do
exp = OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :delta
end

it 'sets parameters from the environment and converts them to symbols' do
exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new
end
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :potato
end

it 'prefers explicit parameters rather than the environment and converts them to symbols' do
exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(aggregation_temporality: 'pickles')
end
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :pickles
end
end

describe '#collect' do
it 'returns all the data points' do
ebh.update(0, {}, data_points)
Expand Down
21 changes: 21 additions & 0 deletions metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,27 @@
let(:start_time) { (Time.now.to_r * 1_000_000_000).to_i }
let(:end_time) { ((Time.now + 60).to_r * 1_000_000_000).to_i }

describe '#initialize' do
it 'defaults to the delta aggregation temporality' do
exp = OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :delta
end

it 'sets parameters from the environment and converts them to symbols' do
exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new
end
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :potato
end

it 'prefers explicit parameters rather than the environment and converts them to symbols' do
exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(aggregation_temporality: 'pickles')
end
_(exp.instance_variable_get(:@aggregation_temporality)).must_equal :pickles
end
end

it 'sets the timestamps' do
sum_aggregation.update(0, {}, data_points)
ndp = sum_aggregation.collect(start_time, end_time, data_points)[0]
Expand Down
Loading