feat: add basic support for metrics exemplar#1609
feat: add basic support for metrics exemplar#1609kaylareopelle merged 35 commits intoopen-telemetry:mainfrom
Conversation
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
…o metrics-exemplar
|
Hi @xuan-cao-swi, this is a big PR and I'm about 40% of the way through it. I'm going to continue my review tomorrow, but I have one small request in the meantime. Could you add documentation to the |
|
@kaylareopelle Thanks! Sorry about this huge PR. I added the doc for exemplar-related params in api. |
@xuan-cao-swi Thanks for the updates! No problem about the size, just takes a little more time :) The upside to big PRs is that they're usually feature-complete, so you can see how everything fits together. Thanks for your patience! |
kaylareopelle
left a comment
There was a problem hiding this comment.
Great job on this! I'm not finished with my review yet. I'm sharing some comments now so you can look at them. I need more time to understand the rest of the PR and run some tests. I will continue the review tomorrow. Thank you!
| bucket_counts: hdp.bucket_counts, | ||
| explicit_bounds: hdp.explicit_bounds, | ||
| exemplars: hdp.exemplars, | ||
| exemplars: hdp.exemplars&.map { |ex| as_otlp_exemplar(ex) } || [], |
There was a problem hiding this comment.
I get a little worried sometimes about calling methods within argument assignments. It might throw off an entire export instead of just impacting that particular key. What do you think about moving the &.map { |ex| as_otlp_exemplar(ex) } || [] logic to a method and passing hdp.exemplars as an arg?
There was a problem hiding this comment.
Refactored to a separate function as_otlp_exemplars.
| ndp.start_time_unix_nano = start_time | ||
| ndp.time_unix_nano = end_time | ||
| reservoir = @exemplar_reservoir_storage[ndp.attributes] | ||
| ndp.exemplars = reservoir&.collect(attributes: ndp.attributes, aggregation_temporality: :delta) |
There was a problem hiding this comment.
Do we want to force the aggregation_temporality to be :delta here? Do we have access to @aggregation_temporality like in the sum aggregation?
There was a problem hiding this comment.
I think we should force :delta here as last_value implements the gauge aggregation which has no aggregation temporality.
| # Adds a new exemplar_filter to replace exist exemplar_filter | ||
| # Default to TraceBasedExemplarFilter | ||
| # | ||
| # @param exemplar_filter the new ExemplarFilter to be added. | ||
| def exemplar_filter_on(exemplar_filter: Exemplar::TraceBasedExemplarFilter) | ||
| @exemplar_filter = exemplar_filter | ||
| end |
There was a problem hiding this comment.
I'm a worried about naming this method exemplar_filter_on. Is that a name used in other implementations?
Just reading the method name, I assume it turns on the AlwaysOn filter.
What do you think about something like enable_exemplar_filter?
There was a problem hiding this comment.
Updated the name to enable_exemplar_filter and disable_exemplar_filter
| module Metrics | ||
| module Exemplar | ||
| # ExemplarFilter | ||
| class ExemplarFilter |
There was a problem hiding this comment.
Since the other exemplar filters inherit from this one, what do you think about changing the code structure to create a subdirectory within exemplar just for filters?
There was a problem hiding this comment.
I'd like to keep them flat (e.g. no subdirectory) like other languages' file structure
| module Exemplar | ||
| # ExemplarReservoir base class | ||
| # Subclasses must implement offer and collect methods | ||
| class ExemplarReservoir |
There was a problem hiding this comment.
Similar question as to what I had for filters -- should there be a subdirectory for the reservoirs that inherit from this one?
| # TraceBasedExemplarFilter | ||
| class TraceBasedExemplarFilter < ExemplarFilter | ||
| def self.should_sample?(value, timestamp, attributes, context) | ||
| ::OpenTelemetry::Trace.current_span(context).context.trace_flags.sampled? |
There was a problem hiding this comment.
I'm a little worried that so many chained method calls could raise an error. Rather than adding safe nav operators to each one, what do you think about adding a rescue that would just catch any error and make the method return false?
There was a problem hiding this comment.
Updated the function that separates the call chain and rescue
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
kaylareopelle
left a comment
There was a problem hiding this comment.
@xuan-cao-swi - Thank you for all your updates!
| end | ||
| end | ||
|
|
||
| # TODO: OpenTelemetry.meter_provider.add_view |
There was a problem hiding this comment.
Do we still need this TODO?
There was a problem hiding this comment.
Sorry, I missed the review. No, we don't need it. I will create another pr to clean this up
| OpenTelemetry.logger = original_logger | ||
| end | ||
|
|
||
| def create_meter |
There was a problem hiding this comment.
You know what, I think it's okay here. I was thinking about moving it to the test_helpers gem originally, but it doesn't seem very useful outside of this test environment on second thought.
And good to know! Thank you for clarifying!
kaylareopelle
left a comment
There was a problem hiding this comment.
Thanks for your patience and work on this feature, @xuan-cao-swi. This is a big PR and though I think I understand it now, I may have missed a few things. I'm comfortable getting this out into the wild and we can address any follow-ups after users test it out.
## Changes Ruby recently merged a PR adding support for Exemplars to our Metrics implementation. See: open-telemetry/opentelemetry-ruby#1609 This PR updates the spec compliance matrix to reflect the change. For non-trivial changes, follow the [change proposal process](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change). * [-] Related issues # * [-] Related [OTEP(s)](https://github.com/open-telemetry/oteps) # * [-] Links to the prototypes (when adding or changing features) * [-] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes * For trivial changes, include `[chore]` in the PR title to skip the changelog check * [X] [Spec compliance matrix](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix/template.yaml) updated if necessary
Description
Exemplar is useful for trace-metric correlation and I'd like to make a contribute (based on otel ticket and otel spec).
Exemplars in Grafana and Tempo (trace)
Exemplars in Prometheus