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

Move FPTI tenant_name into event_params #1104

Merged
merged 6 commits into from
Oct 4, 2023
Merged

Move FPTI tenant_name into event_params #1104

merged 6 commits into from
Oct 4, 2023

Conversation

jaxdesmarais
Copy link
Contributor

Summary of changes

  • Move FPTI tenant_name into event_params - this allows us to search internal tooling with the correct tenant as Braintree (currently it's being overridden to PayPal)

Checklist

  • Added a changelog entry

Authors

@jaxdesmarais jaxdesmarais requested a review from a team as a code owner September 25, 2023 15:01
@scannillo
Copy link
Contributor

Do we also want to change the component name to match the Arachne event migration as well in this PR? Arachne is sending all platforms as braintreeclientsdk

@scannillo
Copy link
Contributor

While we should make this change/fix, it will stink that our previous SDK versions that don't have this update will have analytics in an entirely separate DB table. We should see if Ian can account for this in the current dashboard or if he'll need to make a new BT iOS specific one.

@jaxdesmarais
Copy link
Contributor Author

Do we also want to change the component name to match the Arachne event migration as well in this PR? Arachne is sending all platforms as braintreeclientsdk

@scannillo yeah it makes sense to me to make both of those changes in 1 PR vs having them split across releases. I can make that update here if we want. Though will this make it harder to track mobile specific events and should we make the same change on PPCP (from ppcpmobilesdk to ppcpclientsdk)?

While we should make this change/fix, it will stink that our previous SDK versions that don't have this update will have analytics in an entirely separate DB table. We should see if Ian can account for this in the current dashboard or if he'll need to make a new BT iOS specific one.

Agree it would have been nice to have the arachne > FPTI migration done before so we had some of this in place. But hopefully will make future FPTI migrations easier. Hopefully Ian can help us sort the data out easily.

@scannillo
Copy link
Contributor

Though will this make it harder to track mobile specific events and should we make the same change on PPCP (from ppcpmobilesdk to ppcpclientsdk)?

:sigh: Yeh. I personally would rather have separate component names for: mobile, web, server. It makes the query more efficient vs having to always parse the platform property. And I think that grouping is the most common sub-set of how we query data anyways. I know in stand we decided to use braintreeclientsdk for everything (at least on BT), but maybe we can bring this up again and suggest Arachne migrate to using different components for the 3 platforms? What do you think?

@jaxdesmarais
Copy link
Contributor Author

:sigh: Yeh. I personally would rather have separate component names for: mobile, web, server. It makes the query more efficient vs having to always parse the platform property. And I think that grouping is the most common sub-set of how we query data anyways. I know in stand we decided to use braintreeclientsdk for everything (at least on BT), but maybe we can bring this up again and suggest Arachne migrate to using different components for the 3 platforms? What do you think?

Yeah I totally agree, I think the tenant name makes sense to change because that was a bug. The component I think makes sense to separate out per platform to make searching easier, like you said. That's also an easier change to make imo because the data exists in both FPTI and Arachne so it's less of a "breaking" change with the data existing in 2 places.

@scannillo
Copy link
Contributor

scannillo commented Sep 26, 2023

The component I think makes sense to separate out per platform to make searching easier, like you said. That's also an easier change to make imo because the data exists in both FPTI and Arachne so it's less of a "breaking" change with the data existing in 2 places.

So if we go this route, we'll want to update Arachne to send btwebsdk & btmobilesdk instead of just braintreeclientsdk, right?

(Also hit the wrong key, didn't mean to close the PR)

@scannillo scannillo closed this Sep 26, 2023
@scannillo scannillo reopened this Sep 26, 2023
@jaxdesmarais
Copy link
Contributor Author

So if we go this route, we'll want to update Arachne to send btwebsdk & btmobilesdk instead of just braintreeclientsdk, right?

Yeah I think that makes the most sense unless there is a specific reason we wanted them all together. I would think if we wanted data for all platforms we could have both components set in the tables like we do for the BT + PPCP notebooks Ian made vs having to sort on additional details.

@scannillo
Copy link
Contributor

So if we go this route, we'll want to update Arachne to send btwebsdk & btmobilesdk instead of just braintreeclientsdk, right?

Yeah I think that makes the most sense unless there is a specific reason we wanted them all together. I would think if we wanted data for all platforms we could have both components set in the tables like we do for the BT + PPCP notebooks Ian made vs having to sort on additional details.

Cool - let's loop in @debrado as well

@jaxdesmarais
Copy link
Contributor Author

Adding here for posterity - we decided to move forward with using braintreeclientsdk as the component and sorting on the platform if needed.

@scannillo scannillo merged commit 61c2170 into main Oct 4, 2023
6 checks passed
@scannillo scannillo deleted the update-fpti-tenant branch October 4, 2023 18:47
@jaxdesmarais jaxdesmarais mentioned this pull request Oct 10, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants