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

Migrate ExperimentPayload to AB Testing #563

Merged
merged 3 commits into from
Jun 25, 2019
Merged

Conversation

allisonbm92
Copy link
Contributor

The ExperimentPayload protocol buffer is currently vendored into both Remote Config and In-app Messaging. This problem was masked previously, because Remote Config was obfuscated before release. This change migrates the necessary classes to AB Testing. The same is being performed for In-app Messaging.

@googlebot googlebot added the cla: yes Override cla label Jun 24, 2019
Copy link
Contributor

@miraziz miraziz left a comment

Choose a reason for hiding this comment

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

The ABT proto was deprecated and removed (from ABT) as part of the release of the new SDKs at I/O.

However, the FRC SDK has kept that proto to maintain backwards compatibility with ABT experiments that were stored using the old FRC SDK. The reason for the proto removals was to transition the FRC and ABT SDKs to a JSON format, so I don't think it makes sense to reintroduce the proto dependency to ABT.

Possible solution: since all the experiments that were stored as protos should have expired by now (90 day TTL), we can remove the ABT proto converter from FRC, along with the proto dependency.

@ashwinraghav
Copy link
Contributor

The ABT proto was deprecated and removed (from ABT) as part of the release of the new SDKs at I/O.

However, the FRC SDK has kept that proto to maintain backwards compatibility with ABT experiments that were stored using the old FRC SDK. The reason for the proto removals was to transition the FRC and ABT SDKs to a JSON format, so I don't think it makes sense to reintroduce the proto dependency to ABT.

Possible solution: since all the experiments that were stored as protos should have expired by now (90 day TTL), we can remove the ABT proto converter from FRC, along with the proto dependency.

I think it is a product decision as to whether/not you want to remove the compatibility with legacy configs. I am not sure how the 90 days TTL helps us rule out problems.

If a developer is using a pre IO version of RC and ABT today and decide to switch to the newest version tomorrow, are there experiments that they will lose in the process (if we delete the proto from RC?) ?

@miraziz
Copy link
Contributor

miraziz commented Jun 24, 2019

The ABT proto was deprecated and removed (from ABT) as part of the release of the new SDKs at I/O.
However, the FRC SDK has kept that proto to maintain backwards compatibility with ABT experiments that were stored using the old FRC SDK. The reason for the proto removals was to transition the FRC and ABT SDKs to a JSON format, so I don't think it makes sense to reintroduce the proto dependency to ABT.
Possible solution: since all the experiments that were stored as protos should have expired by now (90 day TTL), we can remove the ABT proto converter from FRC, along with the proto dependency.

I think it is a product decision as to whether/not you want to remove the compatibility with legacy configs. I am not sure how the 90 days TTL helps us rule out problems.

If a developer is using a pre IO version of RC and ABT today and decide to switch to the newest version tomorrow, are there experiments that they will lose in the process (if we delete the proto from RC?) ?

Yea, you're right, I take that back. The TTL will not make a difference in this case.

The current plan is to completely remove the Legacy logic in a future major release, since that will provide everyone with enough time to migrate to post-I/O SDKs.

The ideal end state would be that FRC (and its hard dependency, ABT) does not depend on protos. So, once FRC removes Legacy logic, ABT should remove its proto dependency as well. If FIAM decides to keep using protos after that, then it should convert the values to JSONs or Maps before sending them to ABT.

As part of this change, can we add a TODO(issue/###) to remove the proto dependency from ABT when the Legacy logic in FRC is removed?

@allisonbm92
Copy link
Contributor Author

allisonbm92 commented Jun 25, 2019

The ideal end state would be that FRC (and its hard dependency, ABT) does not depend on protos. So, once FRC removes Legacy logic, ABT should remove its proto dependency as well. If FIAM decides to keep using protos after that, then it should convert the values to JSONs or Maps before sending them to ABT.

As part of this change, can we add a TODO(issue/###) to remove the proto dependency from ABT when the Legacy logic in FRC is removed?

I can link the issue. Does it exist? If not, can you create one? I believe you have more context here.

However, we do need to fix the duplicate proto problem soon, because Remote Config is currently in a unreleasable state.

@miraziz
Copy link
Contributor

miraziz commented Jun 25, 2019

The ideal end state would be that FRC (and its hard dependency, ABT) does not depend on protos. So, once FRC removes Legacy logic, ABT should remove its proto dependency as well. If FIAM decides to keep using protos after that, then it should convert the values to JSONs or Maps before sending them to ABT.
As part of this change, can we add a TODO(issue/###) to remove the proto dependency from ABT when the Legacy logic in FRC is removed?

I can link the issue. Does it exist? If not, can you create one? I believe you have more context here.

However, we do need to fix the duplicate proto problem soon, because Remote Config is currently in a unreleasable state.

Created it with the proper context:
#568

@allisonbm92 allisonbm92 merged commit add8c9b into master Jun 25, 2019
@allisonbm92 allisonbm92 deleted the allison-abt-proto branch June 25, 2019 19:25
schmidt-sebastian pushed a commit that referenced this pull request Jun 27, 2019
* Migrate ExperimentPayload to AB Testing

* Add issue link.

* Export proto definition.
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants