Fix serde

[hwrdtm]

Make compatible with Collector serde shape

Changes

This PR:

• Flattens some data and value fields in order to be consistent with the JSON schema implemented on the OTEL Collector
  • See here for the struct defs and JSON tags
  • Also see here for an example resource metric in JSON
• Make the common::v1::KeyValue::value field serde into an Option<AnyValue> rather than the direct serialized value in order to be consistent with the JSON schema implemented on the OTEL Collector
  • See here for the struct defs and JSON tags
  • Also see here for an example resource span in JSON
  • Similar comment made on an earlier PR: https://github.com/open-telemetry/opentelemetry-rust/pull/1462/files#r1623330058

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hwrdtm / name: Howard (e227ae9, e9556ec, f63c3cc)
• ✅ login: TommyCpp / name: Zhongyang Wu (6645053)

[TommyCpp]

Sorry I saw you comment on my previous PR but didn't get a chance to dig deep and reply 😞 Will take another look today

[lalitb]

@hwrdtm - These proto-generated rust files shouldn't be directly modified. The files are generated using grpc_build.rs, and most probably you need to modify this build script to get the required changes in the proto files.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.5%. Comparing base (9356106) to head (e9556ec).
Report is 1 commits behind head on main.

❗ Current head e9556ec differs from pull request most recent head 6645053

Please upload reports for the commit 6645053 to get more accurate results.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #1856   +/-   ##
=====================================
  Coverage   74.5%   74.5%           
=====================================
  Files        122     122           
  Lines      19809   19809           
=====================================
  Hits       14760   14760           
  Misses      5049    5049           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hwrdtm]

@hwrdtm - These proto-generated rust files shouldn't be directly modified. The files are generated using grpc_build.rs, and most probably you need to modify this build script to get the required changes in the proto files.

@lalitb done, and CI passing now.

[lalitb]

@hwrdtm Had a quick look into the PR. I am not sure what's wrong with the serialize_to_value conversion, as it generates the correct JSON format as here. Or are you specifically fixing the flattening in case of nested attributes?

[hwrdtm]

@hwrdtm Had a quick look into the PR. I am not sure what's wrong with the serialize_to_value conversion, as it generates the correct JSON format as here. Or are you specifically fixing the flattening in case of nested attributes?

@lalitb not sure how the deserialization is working as in this test, but the serialization path is definitely not producing the same result - I implemented a serde test in another PR to demonstrate, check it out for more details.

Also my comment here has an example for what the issue is. Basically, we are currently getting this during serialization:

"value": "100"

when we want

"value": {
  "intValue": 100
}

[TommyCpp]

Not sure if

"value": {
  "intValue": 100
}

is the expected result as pointed out in #1753, int64, fixed64, uint64 should be serialized as string in json.

I think we should fix the serialize_to_value function.

I think we want

"value": {
  "intValue": "100"
}

[hwrdtm]

Not sure if

"value": {
  "intValue": 100
}

is the expected result as pointed out in #1753, int64, fixed64, uint64 should be serialized as string in json.

I think we should fix the serialize_to_value function.

I think we want

"value": {
  "intValue": "100"
}

@TommyCpp From the proto docs

JSON value will be a decimal string. Either numbers or strings are accepted.

So I think either works.

Actually I verified this locally and my OTEL Collector parsed the "intValue": 100 just fine.

Also this example trace in the open-telemetry/opentelemetry-collector uses a mixture of strings for the intValue and numbers for the doubleValue.

Also quick repo search in each of the open-telemetry/opentelemetry-rust and open-telemetry/opentelemetry-collector shows that there is a mixture of string and number values used for intValue:

• open-telemetry/opentelemetry-rust
• open-telemetry/opentelemetry-collector

However, if you feel strongly towards making consistent use of numbers, happy to do that too.

[lalitb]

So I think either works.

Actually I verified this locally and my OTEL Collector parsed the "intValue": 100 just fine.

Yes, either will work. However, JSON parsers may not support the full range of 64-bit integers, so serializing as strings ensures compatibility. Although, during deserialization, we should be able to handle both string and int.

[lalitb]

@hwrdtm - I validated your test in PR #1860 with fix in serialize_to_json here - https://github.com/open-telemetry/opentelemetry-rust/compare/main...lalitb:test-serde-int64?expand=1 , and test is successful.

The change ensures that we serialize i64 as

"value": {
  "intValue": "100"
}

and not

"value": {
  "intValue": 100
}

or

"value": "100"

So the custom serializer is used here to encode i64 as a string. I didn't test it fully, but if the changes look fine, and you can update this PR accordingly?

[lalitb]

as discussed above - #1856 (comment)

[lalitb]

@hwrdtm Do you need any help in incorporating the changes as discussed above, including the unit test from #1860? If it helps, I can create a separate PR for it. #1835 is also blocked on this, so checking.

[hwrdtm]

@lalitb last time I tried looking down the path of changing the logic in the serializers I was having some trouble. I don't think I'll have time to get back to this PR until mid next week - feel free to help get it over the finish line.

[lalitb]

fix incorporated in #1882.

[hwrdtm]

hey @lalitb , thanks for merging in #1882 .

I don't think the other fixes in this PR as referred to here got incorporated though? I am referring to the fixes for these fields:

        "metrics.v1.Metric.data",
        "metrics.v1.NumberDataPoint.value",
        "metrics.v1.Exemplar.value",

[lalitb]

hey @lalitb , thanks for merging in #1882 .

I don't think the other fixes in this PR as referred to here got incorporated though? I am referring to the fixes for these fields:

        "metrics.v1.Metric.data",
        "metrics.v1.NumberDataPoint.value",
        "metrics.v1.Exemplar.value",

@hwrdtm I haven't tested, but do we need them? I believe all these fields use "common::v1::KeyValue" internally, and we already have custom serialization for it. If you foresee any issue, feel free to raise an PR, else I can validate it out later next week.