-
Notifications
You must be signed in to change notification settings - Fork 60
Add support for metrics to emit #767
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
=======================================
- Coverage 75.9% 75.7% -0.3%
=======================================
Files 67 68 +1
Lines 5435 5494 +59
=======================================
+ Hits 4127 4159 +32
- Misses 1308 1335 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lquerel
left a comment
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.
I made few suggestions to better separate metrics from spans but otherwise LGTM
crates/weaver_emit/src/lib.rs
Outdated
|
|
||
| // TODO Emit metrics | ||
| // Emit metrics | ||
| let meter_provider = match tracer_provider_config { |
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.
tracer_provider_config could be renamed/generalized into signal_provider_config
crates/weaver_emit/src/lib.rs
Outdated
| let meter_provider = match tracer_provider_config { | ||
| ExporterConfig::Stdout => init_stdout_meter_provider(), | ||
| ExporterConfig::Otlp { endpoint } => { | ||
| init_meter_provider(endpoint).map_err(|e| Error::TracerProviderError { |
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.
TracerProviderError -> MetricProviderError ?
crates/weaver_emit/src/spans.rs
Outdated
| /// Values are generated based on the attribute type and examples where possible. | ||
| #[must_use] | ||
| fn get_attribute_name_value(attribute: &Attribute) -> KeyValue { | ||
| pub fn get_attribute_name_value(attribute: &Attribute) -> KeyValue { |
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.
Should not that be moved into a dedicated module?
Closes #748 and #752
Emits a metric for every one defined in the model.
Deficiencies:
intorfloat. So integer instruments are created in all cases.1is used in all cases.Bug fixes:
This feature highlighted two issues which are fixed here with regression tests.