Skip to content

Serialization Fails for u64 Fields Due to Incoming String Values in OpenTelemetry Metrics #2487

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.

Sign up for GitHub

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

Jump to bottom
Open
nikhilsinhaparseable opened this issue Dec 30, 2024 · 12 comments · May be fixed by #2491
Open

Serialization Fails for u64 Fields Due to Incoming String Values in OpenTelemetry Metrics #2487

nikhilsinhaparseable opened this issue Dec 30, 2024 · 12 comments · May be fixed by #2491
Assignees
@lalitb

Comments

@nikhilsinhaparseable
Copy link

I am using the opentelemetry-proto crate to serialize incoming OpenTelemetry metrics data. The crate defines certain fields (count, bucket_counts under HistogramDataPoint) as u64. However, the OpenTelemetry exporter sends these fields as strings, which leads to serialization failures.

This mismatch between the data types in the crate and the incoming data prevents successful serialization of metrics.

Sample JSON
otel-metrics-histogram.json

Similar issue is found in the ExponentialHistogramDataPoint
@cijothomas
Copy link
Member

the OpenTelemetry exporter sends these fields as strings, which leads to serialization failures.

Which exporter are you referring to?

@nikhilsinhaparseable
Copy link
Author

I am using otel collector in opentelemetry-demo to send the otel logs, metrics and traces to Parseable.

@lalitb
Copy link
Member

lalitb commented Dec 31, 2024

As per the proto-json conversion standard, u64 should be serialized to string - https://protobuf.dev/programming-guides/json/. So i believe, opentelemetry-proto is doing it correctly?

@nikhilsinhaparseable
Copy link
Author

nikhilsinhaparseable commented Jan 1, 2025 •
edited
Loading

I can see that the serialization to string is happening for the fields where below is available in the crate but this feature is not available for the mentioned fields.

#[cfg_attr( feature = "with-serde", serde( serialize_with = "crate::proto::serializers::serialize_u64_to_string", deserialize_with = "crate::proto::serializers::deserialize_string_to_u64" ) )]

@lalitb
Copy link
Member

lalitb commented Jan 1, 2025

I can see that the serialization to string is happening for the fields where below is available in the crate but this feature is not available for the mentioned fields.

#[cfg_attr( feature = "with-serde", serde( serialize_with = "crate::proto::serializers::serialize_u64_to_string", deserialize_with = "crate::proto::serializers::deserialize_string_to_u64" ) )]

Yes, I did realize it afterwards, and have done changes locally to test before raising PR here -
https://github.com/lalitb/opentelemetry-rust/pull/44/files

@lalitb lalitb linked a pull request Jan 1, 2025 that will close this issue
Open
4 tasks
@nikhilsinhaparseable
Copy link
Author

@lalitb I see similar issue in SummaryDataPoint for the field quantile_values
as the fields quantile and value fields in the ValueAtQuantile struct are f64 data type but the data from the collector is somethings like this -
image

below is the sample JSON with summary metrics -
otel-metrics-summary.json

@lalitb
Copy link
Member

lalitb commented Jan 3, 2025 •
edited
Loading

I see similar issue in SummaryDataPoint for the field quantile_values

@nikhilsinhaparseable Will include fix for this in #2491.

@lalitb lalitb mentioned this issue Feb 5, 2025
Merged
@scottgerring
Copy link
Contributor

scottgerring commented Mar 24, 2025 •
edited
Loading

Is this probably also #2434? It would be great to re-check if this fixes it once the PR goes in; there is some testing in integration tests that could then be properly roundtripped.

@nikhilsinhaparseable
Copy link
Author

@scottgerring i am able to work with the sum metric type

@nikhilsinhaparseable
Copy link
Author

@scottgerring may I know if you were able to work on this one?

@scottgerring
Copy link
Contributor

Hey @nikhilsinhaparseable i've not worked on this, but it looks like @lalitb might have?
I note also that we have a ignored test that roundtrips using the pb models that should be useful for fixing this / asserting that it is fixed:

opentelemetry-rust/opentelemetry-otlp/tests/integration_test/tests/metrics_roundtrip.rs

Lines 27 to 49 in 10cf02c

///
/// Validate JSON/Protobuf models roundtrip correctly.
///
/// TODO - this test fails currently. Fields disappear, such as the actual value of a given metric.
/// This appears to be on the _deserialization_ side.
/// Issue: https://github.com/open-telemetry/opentelemetry-rust/issues/2434
///
#[tokio::test]
#[ignore]
async fn test_roundtrip_example_data() -> Result<()> {
let metrics_in = include_str!("../expected/metrics/test_u64_counter_meter.json");
let metrics: MetricsData = serde_json::from_str(metrics_in)?;
let metrics_out = serde_json::to_string(&metrics)?;
println!("{:}", metrics_out);
let metrics_in_json: Value = serde_json::from_str(metrics_in)?;
let metrics_out_json: Value = serde_json::from_str(&metrics_out)?;
assert_eq!(metrics_in_json, metrics_out_json);
Ok(())
}

@nikhilsinhaparseable
Copy link
Author

@scottgerring the issue i reported in my earlier comment is with Summary metric type where if quantileValues have floating point data, the serialization fails, the same has been discussed with @lalitb in the PR #2491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lalitb lalitb

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@scottgerring @lalitb @cijothomas @nikhilsinhaparseable