json: ser/de bytes as base64 strings not an array of bytes

[adriangb]

I believe this is how the proto <-> json spec defines that binary data should be handled: https://protobuf.dev/programming-guides/json/

I confirmed by exporting data from Python:

from opentelemetry.proto.collector.trace.v1.trace_service_pb2 import ExportTraceServiceRequest
from opentelemetry.proto.common.v1.common_pb2 import AnyValue, KeyValue
from opentelemetry.proto.resource.v1.resource_pb2 import Resource
from opentelemetry.proto.trace.v1.trace_pb2 import ResourceSpans
from google.protobuf.json_format import MessageToJson


export = ExportTraceServiceRequest(
    resource_spans=[
        ResourceSpans(
            resource=Resource(
                attributes=[
                    KeyValue(key="key", value=AnyValue(bytes_value=b"value"))
                ]
            )
        )
    ]
)

json = MessageToJson(export, use_integers_for_enums=True)

print(json)

with open("export.json", "w") as f:
    f.write(json)

{
  "resourceSpans": [
    {
      "resource": {
        "attributes": [
          {
            "key": "key",
            "value": {
              "bytesValue": "dmFsdWU="
            }
          }
        ]
      }
    }
  ]
}

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 74 lines in your changes missing coverage. Please review.

Project coverage is 77.6%. Comparing base (a1dda22) to head (fc2491f).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry/src/common.rs	0.0%	70 Missing ⚠️
opentelemetry-proto/src/transform/common.rs	0.0%	2 Missing ⚠️
opentelemetry/src/metrics/mod.rs	0.0%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2471     +/-   ##
=======================================
- Coverage   77.9%   77.6%   -0.3%     
=======================================
  Files        123     123             
  Lines      22888   22962     +74     
=======================================
  Hits       17839   17839             
- Misses      5049    5123     +74     

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

[adriangb]

I'm not familiar with the codebase so I took a guess as to where to apply the fix. I tried to add an integration test but I could not get a failure case to fail a test so I'm not sure if that's even in the right place either. Some pointers would be appreciated.

[adriangb]

Hi @lalitb @TommyCpp would one of you mind reviewing or pinging an appropriate reviewer (sorry if this is not relevant to you, I just went off of git history)? Thanks

[lalitb]

To make it complete, can we also add this entry in the json samples for logs and metrics?

[adriangb]

Sure yes!

[adriangb]

Added. But if these are the outputs, shouldn't tests be failing?

[adriangb]

Also I wasn't sure how to add an example in a logs body field. Can you suggest the right way to do that?

[lalitb]

Added. But if these are the outputs, shouldn't tests be failing?

Yes these would indeed fail. Please run ./scripts/integration_tests.sh locally to validate that.

ps. - disable the failing tests from #2495 before that, so that you only get failures from your changes.

[lalitb]

Also I wasn't sure how to add an example in a logs body field. Can you suggest the right way to do that?

Good question. The log appender doesn't handle bytes as attribute value for now. The good test probably would be adding binary data to resource, and update json accordingly.

[adriangb]

That's exactly what I did 😄 https://github.com/open-telemetry/opentelemetry-rust/pull/2471/files#diff-4629c7e6add9d0ba42d5f44077d079b4c6f85ac86bc326a42106d67146ae4dbc

[adriangb]

@lalitb I can't seem to get tests to agree on the order of items in attributes arrays. I'm not sure if they're getting sorted in one place but not another or something. Otherwise all tests seem to be passing. I'll point out that I had to implement bytes support for the rust sdk itself, maybe that should be it's own PR but if so it will block this PR.

[adriangb]

Hi, just checking if there's anything else I can do to move this along. Thanks!