Don't Encode wrp again

[kristinapathak]

trafficstars

https://github.com/xmidt-org/caduceus/blob/94286dd77ba28343644de15c5837fcab81255216/outboundSender.go#L519

Instead of re-encoding the wrp, we should just pass along the original bytes that caduceus received and send them in this case.

[kristinapathak]

from this function: https://github.com/xmidt-org/caduceus/blob/master/http.go#L101

I realized that the ContentType of the wrp may not be correct when sending a consumer the msgpack we received from Talaria. @schmidtw, is that acceptable?

[schmidtw]

I think to do this, we will need to make a wrapper object that contains the multiple forms of the object. Should this functionality be something in the wrp-go functionality?

[schmidtw]

@kristinaspring to answer your question about the ContentType, the value it represents differs based on the form the WRP takes in the outgoing message.

In the HTTP form of WRP case the payload of the HTTP is the same as the payload of the WRP. The HTTP.Content-Type needs to describe the payload so it is set to the WRP.Content-Type.

In the case where the WRP form is delivered, the HTTP payload is of type application/msgpack (I think). The WRP.Content-Type is still whatever Parodus sent that describes the WRP.payload contents.

[johnabass]

The HTTP Content-Type should not be the content type of the wrp.payload field. It needs to be the Content-Type of the HTTP entity: either application/json or application/msgpack.

The fixWrp function also adds a transaction uuid. Not all messages require a transaction uuid, e.g. events. That function doesn't check that case before applying the "fix".

[kristinapathak]

Steps forward that we discussed:

1. this issue in wrp-go: https://github.com/xmidt-org/wrp-go/issues/37, and integrating it into caduceus
2. Add a metric to see how often we are modifying the wrp in that function. We may want to move the modification of the wrp into talaria (because in the future talaria will be adding other things and re-encoding the wrp) or just get rid of it altogether.

[kristinapathak]

The content type confusion is solved, it is working as expected currently.

[joe94]

Remaining task:

Add a metric to see how often we are modifying the wrp in that function. We may want to move the modification of the wrp into talaria (because in the future talaria will be adding other things and re-encoding the wrp) or just get rid of it altogether.

[joe94]

Just looked at the modified_wrp_count metric values from production and the majority of WRPs have the transaction UUIDs missing and a few have both that and the WRP.contentType as empty.

As mentioned above, transaction UUIDs only apply to request/response WRP messages, caduceus shouldn't need to fill that in.

As for the WRP messages without a ContentType, that's a tricky one ... but from the code it seems like the contract is to default it to application/json.

From what I gathered in the posts above, caduceus should stop populating the tUUIDs and move the code that corrects the WRP.ContentType to talaria.

[kristinapathak]

The team discussed and decided that we shouldn't be adding transaction UUIDs (which aren't even part of the Simple Event spec). The adding of a default content type when it is empty should be handled by Talaria, which is covered under this issue: https://github.com/xmidt-org/talaria/issues/148

The only thing remaining for this issue is removing the code for the modification of the wrp, including the metric attached to it. This can be done after the functionality is added to Talaria.

[kristinapathak]

The change for the default content type was included in this version of talaria: https://github.com/xmidt-org/talaria/releases/tag/v0.5.9

So we can start on the relevant changes in caduceus. This includes:

• [ ] Removing this function https://github.com/xmidt-org/caduceus/blob/aa1419260f195f95b14c6534f40534c453c684ab/http.go#L103
• [ ] Using a wrphttp.Entity instead of a wrp.Message in order to keep track of both the unmarshaled wrp and the []bytes: https://github.com/xmidt-org/wrp-go/blob/main/wrphttp/requestResponse.go#L13
• [ ] Removing this line to re-encode the wrp and instead use the stored []bytes: https://github.com/xmidt-org/caduceus/blob/main/outboundSender.go#L557

[kristinapathak]

With TransactionUUID being added to the SimpleEvent spec, https://github.com/xmidt-org/talaria/issues/174 should be done and released before the above listed changes are made.