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

feat(message-system): add ab testing message #15281

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

adderpositive
Copy link
Contributor

@adderpositive adderpositive commented Nov 6, 2024

Description

  • Experiments would use a Message System to configure Feature Flags (i.e. there will be no UI to config experiments, deployment of the messaging system will be required - independent from Suite release though).
  • Experiment ID (e.g. rename-trade-section)
    • Multiple Variation IDs (e.g. control, buy-swap) - this may be unlimited, but in reality, we won’t use more than 4 for one experiment
    • Each would have an experiment distribution percentage between 0 - 100%

Example of experiments

experiments property is placed in the top level of config.v[version].json

  "experiments": [
      {
          "conditions": [
              {
                  "environment": {
                      "desktop": ">=24.5.1",
                      "mobile": "!",
                      "web": ">=24.5.1"
                  }
              }
          ],
          "experiment": {
              "id": "3bed56a4-ecd8-4e0f-2910-014b484c2aff",
              "groups": [
                  {
                      "variant": "A",
                      "percentage": 30
                  },
                  {
                      "variant": "B",
                      "percentage": 70
                  }
              ]
          }
      }
  ]

TODO

  • update config.v1.json on the develop and stable - add experiments property

Related Issue

Resolve #15279
Resolve #15067

Testing

This PR was tested separately. More information you can find here - #16145 and https://www.notion.so/satoshilabs/A-B-testing-170dc526060680ab9d5af4e5c9d87876

@adderpositive adderpositive added feature Product related issue visible for end user message-system Updates in secure message system config labels Nov 6, 2024
@adderpositive adderpositive self-assigned this Nov 6, 2024
@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch 6 times, most recently from c16f0f3 to e412009 Compare November 8, 2024 14:28
@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch 5 times, most recently from c5db5a1 to cd14491 Compare November 14, 2024 10:40
@adderpositive adderpositive marked this pull request as ready for review November 14, 2024 11:15
@MiroslavProchazka
Copy link
Contributor

Hello @adderpositive , please let's wait for @tomasklim and let's discuss this in more broader context. Personally I do not like the idea to mix it with message system, so lets discuss.

@adderpositive
Copy link
Contributor Author

Hello @adderpositive , please let's wait for @tomasklim and let's discuss this in more broader context. Personally I do not like the idea to mix it with message system, so lets discuss.

Yes, of course. We have been already discussing the extension of the system. I will go ahead and briefly describe it here.

  1. The first solution was implemented directly into the message-system messages. After brainstorming, I have refactored it to the solution in the PR.
  2. The next solution is to rename the system - message-system can be confusing if it manages more functionality than messages only. Examples of some names toggle-system, push-system, ...
  3. But @matejkriz had an excellent point of view the message system should provide a solution how to safely and unforgettably deliver a message to the Suite at runtime - the message can be distinguished as banner, feature or experiment, etc. It is different than it should display only some messages.

I hope it is understandable a little bit. :)

@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch from cd14491 to 4cb3aa7 Compare November 15, 2024 13:04
@adderpositive
Copy link
Contributor Author

adderpositive commented Nov 18, 2024

Tests are failing because the property experiments doesn't exist in the config.v1.json. It should also be updated on the develop and stable.

Fixed

@trezor trezor deleted a comment from github-actions bot Nov 23, 2024
@tomasklim
Copy link
Member

Tests are failing because the property experiments doesn't exist in the config.v1.json. It should also be updated on the develop and stable.

Please make it optional. So that the app does not crash when experiments are not available at all. We will maybe separate AB testing, feature flags, and message system into 3 systems in the future

docs/features/message-system.md Outdated Show resolved Hide resolved
docs/features/message-system.md Show resolved Hide resolved
docs/features/message-system.md Outdated Show resolved Hide resolved
docs/features/message-system.md Outdated Show resolved Hide resolved
docs/features/message-system.md Outdated Show resolved Hide resolved
packages/suite/src/hooks/experiment/useExperiment.ts Outdated Show resolved Hide resolved
suite-common/message-system/config/config.v1.json Outdated Show resolved Hide resolved
suite-common/message-system/schema/config.schema.v1.json Outdated Show resolved Hide resolved
@adderpositive
Copy link
Contributor Author

Tests are failing because the property experiments doesn't exist in the config.v1.json. It should also be updated on the develop and stable.

Please make it optional. So that the app does not crash when experiments are not available at all. We will maybe separate AB testing, feature flags, and message system into 3 systems in the future

Ok. In my opinion, it’s a good approach to separate messages, features, experiments, etc., but only on the application layer. On the data layer (config.vx.jsw), splitting the config into more parts would make maintenance difficult and confusing.

@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch from 4cb3aa7 to 16b8319 Compare November 25, 2024 11:00
Copy link

github-actions bot commented Nov 25, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch from 4e589d9 to b55b152 Compare January 2, 2025 17:56
@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch from b55b152 to 9e004f9 Compare January 3, 2025 09:57
@adderpositive adderpositive force-pushed the feat/ab-testing-on-message-system branch from 9e004f9 to 8df54fe Compare January 6, 2025 16:03
@adderpositive
Copy link
Contributor Author

@matejkriz After our discussion, I removed strict validation. Additionally, you can see a description below if the sum does not equal 100. I hope this helps with your review. 🙃 Changes here c83ddc5

If an array of groups is empty the defaultComponent (first in components) will be displayed.
If the sum of the items in the group is more than 100, the percentage created from instanceId will assigned to the matching group.
If the sum of the items in the group is less than 100 and the percentage created from instanceId does not match any group (e.g. A - 30%, B - 30%, user percentage - 69), it will return defaultComponent (first in components).

@tomasklim tomasklim added the release Will be included in the upcoming release. Needs to be backported to the release branch. label Jan 7, 2025
@tomasklim
Copy link
Member

/rebase

Copy link

github-actions bot commented Jan 7, 2025

@trezor-ci trezor-ci force-pushed the feat/ab-testing-on-message-system branch from c83ddc5 to f1a6ab8 Compare January 7, 2025 12:47
@tomasklim tomasklim merged commit fc56ea0 into develop Jan 7, 2025
33 of 34 checks passed
@tomasklim tomasklim deleted the feat/ab-testing-on-message-system branch January 7, 2025 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Product related issue visible for end user message-system Updates in secure message system config release Will be included in the upcoming release. Needs to be backported to the release branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend message system to include AB testing variant AB Testing capability
4 participants