GoogleCloudPlatform / opentelemetry-operations-go

Apache License 2.0
134 stars 103 forks source link

Exporting log records with a non-JSON Byte body fails #689

Closed BinaryFissionGames closed 1 year ago

BinaryFissionGames commented 1 year ago

If the pdata log record is Bytes, we see this error in our logs:

{"level":"warn","ts":"2023-07-27T14:02:52.373-0400","caller":"batchprocessor@v0.81.0/batch_processor.go:258","msg":"Sender failed","kind":"exporter","data_type":"logs","name":"googlecloud/gcloudtest","error":"logging: json.Unmarshal: json: cannot unmarshal string into Go value of type map[string]interface {}"}

It looks like here, the body is extracted as an []bytes: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/f2188d1e6cfb3c1be26021962ae8f03eb42b916b/exporter/collector/logs.go#L539-L540

Then the payload is put directly on the entry: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/f2188d1e6cfb3c1be26021962ae8f03eb42b916b/exporter/collector/logs.go#L530

However, the docs for logging.Entry states that payload must be string or marshal to a JSON object (not a raw value), which []byte does not do.

damemi commented 1 year ago

Can you share a sample input to get this error? It seems more like a json formatting problem that the call to json.Unmarshal is failing on.

The exact wording in the logging.Entry docs say Payload must be either a string, or something that marshals via the encoding/json package to a JSON object. Raw bytes are the input to encoding/json.Unmarshal, so I think this doc means "what's represented by the bytes must marshal to a JSON object" (ie, not a string or invalid json)

You can see what I mean in this example: https://go.dev/play/p/CRtRb3Uga2_R

We also have a couple unit tests that use byte payloads, though I don't think we have an integration test which would catch if the error was coming from GCM.

BinaryFissionGames commented 1 year ago

The body on these logs were []byte("eyJuYW1lIjogIm5hbWUiLCJ0ZXN0IjogeyJ0ZXN0IjogMSwidGVzdDIiOiAidHdvIn19") - is there an assumption that the body will be a json formatted byte string? I don't think that's an assumption of OTLP.

These logs were retrieved from the kafka receiver with the raw encoding option.

damemi commented 1 year ago

Right, OTLP allows the body to be any, but the GCM payload needs to be a string, or a JSON object (or the byte representation of a JSON object). You might be able to use the transform processor to format it to an object

BinaryFissionGames commented 1 year ago

Yeah, I could finagle the data to work for the exporter, for sure!

But my expectation would be that the bytes would be converted to string (probably encoded somehow), and sent as a text-payload, instead of rejecting the payload with a vague error message.

I personally would not expect the exporter to try and json parse the bytes payload, it seems inconsistent with the behavior for the string payload, which has no extra parsing done to it.

I think at the very least, a better error message and documentation of this behavior would be nice to have.

damemi commented 1 year ago

Ah I see what you mean, yes we could probably make the exporter smarter here to do what you're saying.

Instead of just blindly passing along the raw bytes and letting it fail, we could first try to marshal the bytes ourselves and pre-verify that it is a json object. If so, then we can pass along the bytes.

If not, then the exporter could log a better error to the user (right now I think it's just the logging.Entry proto library that's returning this cryptic error). Then we could convert that to a string payload and pass it along that way. This would probably need to be behind a setting or feature flag, because some users might not want malformed objects to be sent along as strings by default.

damemi commented 1 year ago

Opened https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/691 to show what I'm thinking could work for this

BinaryFissionGames commented 1 year ago

I like that solution, seems great!