-
Notifications
You must be signed in to change notification settings - Fork 250
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 experimental otlp metrics exporter #1551
feat: add experimental otlp metrics exporter #1551
Conversation
The jaeger CI failure is now fixed on |
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.
This is awesome, Xuan! The exporter worked great with a local OTLP collector.
Initially, I had some issues with New Relic's OTLP endpoint. Adding the aggregation_temporality
attribute to the protobuf payload resolved the problem. I have a few suggestions below that add aggregation_temporality
to the exporter. They're accompanied by #1554. That PR adds aggregation_temporality
to MetricData
so that it can be accessed using metrics.aggregation_temporality
.
exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics_exporter.rb
Outdated
Show resolved
Hide resolved
exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics_exporter_test.rb
Outdated
Show resolved
Hide resolved
exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics_exporter_test.rb
Outdated
Show resolved
Hide resolved
exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics_exporter_test.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…exporter.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…_exporter_test.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…_exporter_test.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…exporter.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…elemetry-ruby into metrics-exporter-otlp
@kaylareopelle Thanks for the review, I have added the change based on aggregation_temporality, also added the gem test to ci.yml. |
spec.add_dependency 'opentelemetry-metrics-api' | ||
spec.add_dependency 'opentelemetry-metrics-sdk' |
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.
Probably should remove these dependencies?
Since there is no metrics-related gem in rubygems, user who install this and use it will fail.
User who want to use it should create the metrics gem by themselves, and require metrics gem first, and then this metrics exporter.
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.
Removed the dependencies and included instruction in README that install these two dependencies.
…o metrics-exporter-otlp
…elemetry-ruby into metrics-exporter-otlp
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.
If there are no objections, I'll merge this at EOD. Thanks @xuan-cao-swi.
Follow-up PR for #1545 (move experimental feature into separate gem)
Description
Added otlp metrics exporter that works with otlp collector through http protocol.
Most code copied from (trace) exporter (with test as well) with the assumption of same ssl, http etc. configuration. The major change is the proto decode. If the log exporter also use the similar configuration, having a utility file can help DRY.
Quick demo
Testing failed but it will success once the #1532 is merged.