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.

Sign up for GitHub

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

Jump to bottom

Netx research and refactoring in Sprint 15 #646

Closed
3 tasks done
bassosimone opened this issue May 28, 2020 · 1 comment
Closed
3 tasks done

Netx research and refactoring in Sprint 15 #646

bassosimone opened this issue May 28, 2020 · 1 comment
Assignees
@bassosimone
Labels
effort/M Medium effort priority/medium Medium priority
Milestone
Sprint 15 - Globster

Comments

@bassosimone
Copy link
Contributor

bassosimone commented May 28, 2020 •
edited
Loading

Following up from #576

  • preliminary work for rewriting experiment/telegram using experiment/urlgetter
  • errorx: make sure we correctly handle "context canceled" so it gets scrubbed
  • reimplement experiment/telegram in terms of experiment/urlgetter

This refactoring is functional to measure performance metrics. In turn, by doing that we will be able to measure HTTP fetching performance metrics for Psiphon. This is also a precondition to rewrite Web Connectivity in Go.
@bassosimone bassosimone added this to the Sprint 15 - Globster milestone May 28, 2020
@bassosimone bassosimone self-assigned this May 28, 2020
@bassosimone bassosimone mentioned this issue May 28, 2020
Closed
4 tasks
@bassosimone bassosimone added effort/M Medium effort priority/medium Medium priority labels Jun 8, 2020
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: add tcp_connect to test_keys
edaf401 
This was planned inside #656.

It actually also helps with #646.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: add tcp_connect to test_keys (#675)
a0fff21 
This was planned inside #656.

It actually also helps with #646.
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: allow to configure the method
b352177 
We need this to reimplement the telegram experiment in terms of
the urlgetter experiment, to simplify the code.

See #646.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: allow to configure the method (#676)
a3de377 
We need this to reimplement the telegram experiment in terms of
the urlgetter experiment, to simplify the code.

See #646.
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: introduce the Multi urlgetter
180bfb1 
With the Multi urlgetter you can perform operations in parallel with a
specific degree of parallelism. You get results back as they arrive via
a channel that is returned on output.

We'll use this functionality pretty much soon to rewrite the Telegram
experiment in terms of the urlgetter experiment.

Reference issue: #646
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: introduce the Multi urlgetter (#677)
d6316d6 
With the Multi urlgetter you can perform operations in parallel with a
specific degree of parallelism. You get results back as they arrive via
a channel that is returned on output.

We'll use this functionality pretty much soon to rewrite the Telegram
experiment in terms of the urlgetter experiment.

Reference issue: #646
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx: use constants to represent all operations
527baa4 
This is less error prone. I consider this yak shaving to make the
code more robust before continuing with #646.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx: use constants to represent all operations (#678)
9ead0e1 
This is less error prone. I consider this yak shaving to make the
code more robust before continuing with #646.

While there reduce linter warnings.
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx/archival: extract the operation that failed
7008c0d 
This will be useful to more easily classify the cause of blocking.

This is part of #646
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx/archival: extract the operation that failed (#679)
e1297dc 
This will be useful to more easily classify the cause of blocking.

This is part of #646
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: expose the operation that failed
a939a45 
This is very useful to quickly understand what actually went wrong. It is
a facet of the next design that was never really used.

Also, acknowledge that, in some cases, there isn't a real failed operation
because the error happens before we enter into our network stack.

This happens, for example, when the context we're using is canceled.

To address this potential issue, introduce and use a `"top_level"` operation
that describes this specific situation.

This work is part the telegram rewrite: #646.

Also, with this diff landing we can close #311,
which mentioned oonimkall (the old implementation of netx) but is still
basically talking about the same concept we're implementing here.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter: expose the operation that failed (#680)
746812d 
This is very useful to quickly understand what actually went wrong. It is
a facet of the next design that was never really used.

Also, acknowledge that, in some cases, there isn't a real failed operation
because the error happens before we enter into our network stack.

This happens, for example, when the context we're using is canceled.

To address this potential issue, introduce and use a `"top_level"` operation
that describes this specific situation.

This work is part the telegram rewrite: #646.

Also, with this diff landing we can close #311,
which mentioned oonimkall (the old implementation of netx) but is still
basically talking about the same concept we're implementing here.
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter.TestKeys: add extra fields to simplify analysis
77b2024 
We often need to check the status code of the last response in the chain
and/or to see the body of such response.

To simplify these use cases, introduce two new variables that contain such
values. These variables are not JSON serialised, because it does not make
sense for us to do that. One can always recompute these fields, and the point
of this patch is just to avoid duplicating code.

Added an extra integration test to make sure that the value of these new
variables is okay not only when the context is canceled but also when we're
not doing HTTP but something else, e.g., just TLS.

Part of #646.

We will use this new code to simplify post processing of Telegram results.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
urlgetter.TestKeys: add extra fields to simplify analysis (#681)
07ac3a9 
We often need to check the status code of the last response in the chain
and/or to see the body of such response.

To simplify these use cases, introduce two new variables that contain such
values. These variables are not JSON serialised, because it does not make
sense for us to do that. One can always recompute these fields, and the point
of this patch is just to avoid duplicating code.

Added an extra integration test to make sure that the value of these new
variables is okay not only when the context is canceled but also when we're
not doing HTTP but something else, e.g., just TLS.

Part of #646.

We will use this new code to simplify post processing of Telegram results.
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx/errorx: wrap context.Canceled
bd09875 
Like for MK errors, use the same naming of the C++11 library.

This error will happen when the mobile app will interrupt specific
tests, so better to have a clear naming for it.

Part of #646.
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit to ooni/spec that referenced this issue Jun 9, 2020
@bassosimone
df-007-errors.md: document the interrupted error
c545695 
Introduced in ooni/probe-engine#682

Part of ooni/probe-engine#646
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
netx/errorx: wrap context.Canceled (#682)
bb83b5d 
Like for MK errors, use the same naming of the C++11 library.

This error will happen when the mobile app will interrupt specific
tests, so better to have a clear naming for it.

Part of #646.
bassosimone added a commit to ooni/spec that referenced this issue Jun 9, 2020
@bassosimone
df-007-errors.md: document the interrupted error (#193)
8d103f5 
Introduced in ooni/probe-engine#682

Part of ooni/probe-engine#646
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
telegram: rewrite using experiment/urlgetter
d7bbe88 
Part of #646
This was referenced Jun 9, 2020
Merged
Merged
bassosimone added a commit that referenced this issue Jun 9, 2020
@bassosimone
telegram: rewrite using experiment/urlgetter (#683)
0a0028e 
Part of #646

Passes all checks from ooni/jafar#28
@bassosimone bassosimone mentioned this issue Jun 9, 2020
Closed
1 task
@bassosimone
Copy link
Contributor Author

We have now refactored Telegram. More work in next sprint: #684.

bassosimone added a commit to ooni/jafar that referenced this issue Jun 9, 2020
@bassosimone
Merge pull request #28 from ooni/feature/updatedeps
fe4cc68 
Update deps and adapt to current miniooni

Part of ooni/probe-engine#646

Used to validate ooni/probe-engine#683
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
effort/M Medium effort priority/medium Medium priority
Projects
None yet
Milestone
Sprint 15 - Globster
Development

No branches or pull requests
1 participant
@bassosimone