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.

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Feb 22, 2024

Description

I'd like to contribute on metrics view. I think view will be helpful for customized aggregation over default aggregation on instrumenting metrics.

  1. Changed the location of data_points to metric_stream, because I think aggregation should act as aggregator that provide transformative (on data_points) feature rather than storage feature.
  2. Didn't implement the logic for conflicting metric identities. For current implementation, it will export all the metrics with different aggregation if the metric_stream has multiple views applied.
  3. Need schema_url as part of instrumentation selection. feat: Add basic support for schema_url #1237

Copy link
Contributor

👋 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 keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@kaylareopelle
Copy link
Contributor

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.

@xuan-cao-swi
Copy link
Contributor Author

Thanks for catching it. I use flatten! to ensure it return [MetricData]. I remembered that I made this mistake while I was thinking about the case for multiple view for one metric stream.

@kaylareopelle
Copy link
Contributor

Thanks for catching it. I use flatten! to ensure it return [MetricData]. I remembered that I made this mistake while I was thinking about the case for multiple view for one metric stream.

Thanks for the fix! What else did you want to accomplish before moving the PR out of draft mode?

@xuan-cao-swi
Copy link
Contributor Author

Not in my current mind. I will open it for review

xuan-cao-swi and others added 3 commits July 18, 2024 10:47
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
@xuan-cao-swi
Copy link
Contributor Author

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 Commit suggestion).

xuan-cao-swi and others added 3 commits July 22, 2024 18:23
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
xuan-cao-swi and others added 5 commits September 10, 2024 12:03
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
Copy link
Contributor

@kaylareopelle kaylareopelle left a 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! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Review
Status: Placeholder
Development

Successfully merging this pull request may close these issues.

2 participants