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

oonimkall: investigate using richer input #2752

Closed
bassosimone opened this issue Jun 17, 2024 · 5 comments
Closed

oonimkall: investigate using richer input #2752

bassosimone opened this issue Jun 17, 2024 · 5 comments
Assignees
Labels
cleanup There's need to cleanup stuff a bit enhancement improving existing code or new feature funder/drl2022-2024 ooni/probe-engine priority/high techdebt This issue describes technical debt

Comments

@bassosimone
Copy link
Contributor

This issue is about investigating what it would take for oonimkall to use richer input.

@bassosimone bassosimone self-assigned this Jun 17, 2024
@bassosimone bassosimone added enhancement improving existing code or new feature priority/high ooni/probe-engine techdebt This issue describes technical debt cleanup There's need to cleanup stuff a bit funder/drl2022-2024 labels Jun 19, 2024
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 19, 2024
This diff, which is part of ooni/probe#2752,
prints the event occurring when testing, which is useful for me to better
understand what is going on while changing the package.

Extracted from #1620.
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 19, 2024
This diff, which is part of ooni/probe#2752,
prints the event occurring when testing, which is useful for me to
better understand what is going on while changing the package.

Extracted from #1620.
@bassosimone
Copy link
Contributor Author

bassosimone commented Jun 20, 2024

I discussed this topic extensively with @DecFox. Here's a summary of the conversation:

  1. Running experiments from mobile uses ./pkg/oonimkall/task.go, which is still quite close to the original C interface exposed by Measurement Kit. Under this API, we start tasks using JSON configuration, read JSON events emitted by the task, and stop when the task signals that it is done. The configuration only includes an Input []string. This means that we are not able to correctly run OONI Run v2 descriptors containing options.

  2. Mobile code uses a separate Go-Java bridge generated using go mobile for fetching Web Connectivity URLs. This really only happens for Web Connectivity and all other tests are executed without any input. This means that, if an experiment needs input and there is no input, the oonimkall code will then be responsible for somehow fetching such input.

  3. In fact, oonimkall has specific code that uses the experiment's input policy to determine what to do. This code does not currently invoke any Loader, as explained in a comment, because we were performing this change close to a release and we did not want to introduce too many breaking changes at the time. However, because Web Connectivity already provides URLs, it seems reasonable that we just use a loader there if there's no other input already.

  4. By doing this, we will unlock the possibility of fetching richer input for oonimkall experiments except for Web Connectivity, which uses a different pattern where the app is responsible for fetching inputs.

Because the change seems safe on paper, I propose we implement and evaluate it. Then, we should definitely document as technical debt that the mobile app uses a different pattern than the CLI and that the mobile app does not allow to pass options from OONI Run v2 to experiments. I see both problems as being linked, even though they are different problems, because they break the following narrative, which I think is very good to reason about OONI:

  1. OONI Run v2 is a way to automate running miniooni from the command line.

  2. Richer input is a way to fetch inputs and options when OONI Run v2 does not contain inputs and options.

  3. Experiment execution uses an experiment name, input, and options.

Instead, mobile is kind of different and does not really follow this code organization. However, it is also true that the issues I am describing are not impacting OONI operationally right now. Hence, my suggestion that, for now, we should just focus on unlocking using richer input for dnscheck et al., and document the rest as technical debt.

@bassosimone
Copy link
Contributor Author

So, after #2752 (comment), we were left with the reckoning that there are two richer-input and/or OONI-Run-v2 lingering issues in the ./pkg/oonimkall codebase. These issues are:

Issue 1: It's not possible to pass structured options inside an OONI Run v2 descriptor to an experiment through the ./pkg/oonimkall API, because there's only room for Input []string.

Issue 2: that the pattern for executing experiments used by the mobile apps is not the same used by the CLI or miniooni and we end up not being able to obtain richer input for Web Connectivity easily. This happens because richer input is an interface inside the ./internal part of the codebase but the experiment needs to cast such an interface to the proper type containing richer input. The mobile app would not easily be able to construct the proper type due to difficulties caused by using JSON as the way to pass structures between the mobile app and the engine proper.

Issue 1 can be easily solved by introducing a json.RawMessage field on the receiver side that can receive a raw JSON message produced by the mobile app based on the content of the OONI Run v2 descriptor. So, the solution to this problem is actually quite simple and it should not require too much work.

Issue 2 is more complex to solve. While discussing this topic, @aanorbel proposed what I consider a very good solution of this problem that we should definitely explore. Without changing much in the mobile app, we can just take advantage of the fact that the status.measurement_start event provides the URL we're about to measure to the mobile app. Now, the mobile app also needs to now the category code and the country code, which we can easily supply because they are part of the richer input model used for any richer input. Thus, if we enrich the status.measurement_start event with category code and country code, the mobile app does not need to invoke check-in itself and we can instead invoke check-in inside the ./pkg/oonimkall codebase through a Loader. By implementing this chance, we would factor common code, simplify the mobile apps implementations, and streamline how we execute experiments without any functional changes.

@bassosimone
Copy link
Contributor Author

I also need to remember to add a custom timeout for fetching targets in oonimkall when I modify it.

@bassosimone
Copy link
Contributor Author

bassosimone commented Jun 21, 2024

So, wrapping up and preparing for closing this issue. Here's what should happen next:

We can close this issue when all these three issues have been created. Luckily all three activities seem ~simple. 💥

bassosimone added a commit to ooni/probe-android that referenced this issue Jun 21, 2024
This PoC should show that we can save measurements in the
status.measurement_started callback. Obviously, a complete
diff for probe-android should be more comprehensive and
TBH my aim here is just to show it is *possible*.

Related to ooni/probe#2752.
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 28, 2024
This diff contains a trivial rename of fakeSuccessfulRun back
to fakeSuccessfulDeps. I made the rename from Deps to Run in a
previous commit, but now I realize it was a mistake.

Because this is a trivial patch, I am going to self merge it.

Part of ooni/probe#2752.
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 28, 2024
This diff contains a trivial rename of fakeSuccessfulRun back to
fakeSuccessfulDeps. I made the rename from Deps to Run in a previous
commit, but now I realize it was a mistake.

Because this is a trivial patch generated with a tool, I am going to
self merge it w/o review.

Part of ooni/probe#2752.
@bassosimone
Copy link
Contributor Author

All follow-up issues created. We can close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup There's need to cleanup stuff a bit enhancement improving existing code or new feature funder/drl2022-2024 ooni/probe-engine priority/high techdebt This issue describes technical debt
Projects
None yet
Development

No branches or pull requests

1 participant