Skip to content
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.

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

Don't Encode wrp again #174

Open
kristinapathak opened this issue Oct 28, 2019 · 11 comments
Open

Don't Encode wrp again #174

kristinapathak opened this issue Oct 28, 2019 · 11 comments

Comments

@kristinapathak
Copy link
Contributor

encoder.Encode(msg)

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

@kristinapathak kristinapathak self-assigned this Oct 28, 2019
@kristinapathak
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Collaborator

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
Copy link
Contributor Author

Steps forward that we discussed:

  1. this issue in wrp-go: Add a way to get the original bytes of the wrp wrp-go#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
Copy link
Contributor Author

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

@joe94
Copy link
Member

joe94 commented Apr 6, 2020

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
Copy link
Member

joe94 commented Jul 24, 2020

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
Copy link
Contributor Author

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:
xmidt-org/talaria#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
Copy link
Contributor Author

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:

@kristinapathak
Copy link
Contributor Author

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

@kristinapathak kristinapathak removed their assignment Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants