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

fix: message properties should contain reqType instead of messageID [PIPE-1557] #35

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BonapartePC
Copy link

@BonapartePC BonapartePC commented Sep 24, 2024

Description

message properties should contain reqType instead of messageID since ingestion will no longer have messageID and gw infers it. Also, reqType should be passed to gw.

Linear Ticket

https://linear.app/rudderstack/issue/PIPE-1557/payload-should-not-be-modified-in-ingestionsvc

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@BonapartePC BonapartePC changed the title fix: message properties should contain reqType instead of messageID fix: message properties should contain reqType instead of messageID [PIPE-1557] Sep 24, 2024
Copy link
Collaborator

@fracasula fracasula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the GW already infers the messageID if it's not there? I'm talking about the internal batch endpoint that the src-router hits.

I'm asking to make sure we can release these changes in a backwards compatible manner.

I'm assuming that at the moment the GW expects a messageID and does not expect a request type in the properties. If that's the case then we would need a release of rudder-server first that infers the messageID (unless it already does) and it checks for request type in both the properties and the body. This way we can rollback the HA ingestion services without having to rollback the GW. Once all is released (and stable) we can then drop the extra checks in the GW. Wdyt?

@Sidddddarth
Copy link
Member

Sidddddarth commented Sep 24, 2024

Gateway simply sets the request type to internalBatch in this case and that is used everywhere(only stats now).

Does the GW already infers the messageID if it's not there? I'm talking about the internal batch endpoint that the src-router hits.

And messageID is used to fill the parameters, which isn't even used in processor tbh, it's fetched from the payload. so imo it's fine if it's not populated.

For the request type, we'd have to release server - but only for the purpose of stats, the pipeline can go on.

@fracasula
Copy link
Collaborator

@Sidddddarth OK cool so it sounds like we need a few GW changes first. Not just for stats though.
I'm saying this because if the request type is coming from the properties (this schemas PR) then single events (e.g. track) won't be wrapped around {"batch":[<EVENT>]}. The GW needs to account for that too.

Copy link
Collaborator

@fracasula fracasula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's not merge until we have a proper release of rudder-server that is compatible with both versions.

@fracasula fracasula added the DO NOT MERGE Approved but need to check more label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Approved but need to check more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants