beam-telemetry / telemetry_metrics_prometheus_core

Core Prometheus Telemetry.Metrics Reporter package for telemetry_metrics_prometheus
Apache License 2.0
35 stars 30 forks source link

Move buckets definition to reporter options #22

Closed nathancyam closed 4 years ago

nathancyam commented 4 years ago

Due to changes on the telemetry_metrics repository, which can be found here: https://github.com/beam-telemetry/telemetry_metrics/commit/cc9b30952d36ac8506a2fba30b3905c990de0a7b, the :buckets key as part of the Metrics.Distribution struct has been dropped. This would cause the call to distribution to fail:

iex> Metrics.distribution(
  "http.request.duration.seconds",
  buckets: [0.01, 0.025, 0.05, 0.1, 0.2, 0.5, 1],
  event_name: [:http, :request, :complete],
  measurement: :duration,
  unit: {:native, :second}
)
** (KeyError) key :buckets not found in: %{__struct__: Telemetry.Metrics.Distribution, buckeets: [0.01, 0.025, 0.05, 0.1, 0.2, 0.5, 1], description: nil, event_name: [:broadway, :processor, :stop], keep: nil, measurement: #Function<3.23735626/1 in Telemetry.Metrics.maybe_convert_measurement/2>, name: [:broadway, :processor, :stop, :duration, :seconds], reporter_options: [], tag_values: #Function<0.23735626/1 in Telemetry.Metrics.default_metric_options/0>, tags: [], unit: :second}

As part of the commit above, it suggests moving the :buckets definition to the reporter_options keyword, which this PR aims to address.

codecov-commenter commented 4 years ago

Codecov Report

Merging #22 into master will decrease coverage by 1.64%. The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   99.53%   97.89%   -1.65%     
==========================================
  Files           9        9              
  Lines         216      237      +21     
==========================================
+ Hits          215      232      +17     
- Misses          1        5       +4     
Flag Coverage Δ
#1_10_otp21 97.89% <82.60%> (-1.65%) :arrow_down:
#1_10_otp22 97.89% <82.60%> (-1.65%) :arrow_down:
#1_6_otp20 97.89% <82.60%> (-1.65%) :arrow_down:
#1_6_otp21 97.89% <82.60%> (-1.65%) :arrow_down:
Impacted Files Coverage Δ
lib/core/distribution.ex 100.00% <ø> (ø)
lib/core/registry.ex 93.05% <81.81%> (-4.99%) :arrow_down:
lib/core/aggregator.ex 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c3cc031...745ea93. Read the comment docs.

nathancyam commented 4 years ago

I've added the bucket validation back into registry.ex as well as the unit tests. I've also introduced a new buckets type spec as well. I think that's everything. Just a quick question though, how should we version this? Would we call this a breaking change?

bryannaegele commented 4 years ago

Would we call this a breaking change?

It is a breaking change. There's at least one other change that will need to be made for the next release. Don't sweat that though. It's expected.