-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: add basic metrics view #1604
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 |
As a continuation from our discussion during the SIG on July 9, 2024: I was experimenting with this branch to create an explicit bucket histogram with custom buckets. It worked great! Thank you for building this! The only error I've encountered was while exporting the metrics via OTLP. The structure of the metrics object passed as an argument to the export method has changed. Before: Array[MetricData, MetricData] In this branch: Array[[MetricData],[MetricData]] We could handle this in the exporter, but it may be preferable to go back to the original structure. As far as I can see in the spec, it just needs to be some kind of list, so either way seems compliant. |
Thanks for catching it. I use |
Thanks for the fix! What else did you want to accomplish before moving the PR out of draft mode? |
Not in my current mind. I will open it for review |
metrics_sdk/lib/opentelemetry/sdk/metrics/view/registered_view.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_stream.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
metrics_sdk/lib/opentelemetry/sdk/metrics/view/registered_view.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/view/registered_view.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_stream.rb
Outdated
Show resolved
Hide resolved
Thanks @kaylareopelle, I updated the suggestions. I didn't do commit the suggestions on the github page to avoid github freeze on my browser and create commit issue (I had bad experience last time click each |
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
metrics_sdk/lib/opentelemetry/sdk/metrics/view/registered_view.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
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.
Thank you for adding this and for bearing with me through all the back and forth! 😄
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.
Just a couple of questions, nothing blocking.
Description
I'd like to contribute on metrics view. I think view will be helpful for customized aggregation over default aggregation on instrumenting metrics.
data_points
tometric_stream
, because I think aggregation should act as aggregator that provide transformative (ondata_points
) feature rather than storage feature.