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

enhancement: add gauge metric behind feature #117

Closed
wants to merge 1 commit into from

Conversation

academiaresf
Copy link
Contributor

Motivation

The opentelemetry crate actually supports Gauge metric behind otel_unstable feature.

Solution

Extend the current implementation with the treatment for the new Gauge metric.

The tests are passing but i don't know if there are necessary other type of tests (at this point, the property start_time inside the DataPoints of the Gauge type are None in all the test, for example. L664 of tests/metrics_publishing.rs file).

@biryukovmaxim
Copy link
Contributor

Any chance it will be merged?

Copy link
Collaborator

@jtescher jtescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but the checks didn't run for some reason? Mind pushing an empty commit?

async fn i64_gauge_with_attributes_is_exported() {
let (subscriber, exporter) = init_subscriber(
"hello_world".to_string(),
InstrumentKind::Counter,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these tests use Gauge?

This shouldn't even work because the metric shouldn't be downcastable to Sum or maybe I'm missing something there, but this seems suspicious to me.

I think it would be best to add a test that calls tracing::info twice with different values to test that the value is overwritten and not summed like for counter metrics.

@biryukovmaxim
Copy link
Contributor

Hello guys, I made an another pr based on the branch
there are some tests fixes, guard them by feature, add --all-features flag to cicd.
I hope I didn't offend you, @academiaresf . You did a nice job!

@djc djc mentioned this pull request May 24, 2024
@biryukovmaxim
Copy link
Contributor

biryukovmaxim commented May 24, 2024

since #129 is merged - this one can be closed

@djc djc closed this May 24, 2024
@djc
Copy link
Collaborator

djc commented May 24, 2024

@academiaresf thanks for jumpstarting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants