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: add TunnelTime metric #171

Merged
merged 43 commits into from
Mar 27, 2024
Merged

feat: add TunnelTime metric #171

merged 43 commits into from
Mar 27, 2024

Conversation

sbruens
Copy link

@sbruens sbruens commented Mar 14, 2024

Over a given timeframe, we observe the duration that a given IPKey pair is sending traffic through the server (i.e. how long is it active) and measure it, aggregated by key or location. This will allow service providers to measure TunnelTime.

@sbruens sbruens marked this pull request as ready for review March 15, 2024 22:17
@sbruens sbruens requested a review from a team as a code owner March 15, 2024 22:17
@sbruens sbruens requested a review from fortuna March 15, 2024 22:17
@sbruens
Copy link
Author

sbruens commented Mar 15, 2024

Actually just realized I need to still put this behind a flag, so reverting to draft. But feel free to leave comments on what's there already.

@sbruens sbruens marked this pull request as draft March 15, 2024 22:20
@sbruens sbruens marked this pull request as ready for review March 18, 2024 19:56
Copy link

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

I think we can move some things around a bit. We can also lean more on the "Tunnel Time" name. And we need to make the code concurrency-safe.

cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/main.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/main.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
@sbruens sbruens changed the title feat: add IPKey time metric feat: add TunnelTime metric Mar 20, 2024
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Show resolved Hide resolved
@sbruens
Copy link
Author

sbruens commented Mar 25, 2024

Made some big changes to use the Prometheus Collector interface (thanks for the pointer, much better).

@sbruens sbruens requested a review from fortuna March 25, 2024 18:38
Copy link

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Thanks, this looks much cleaner. I think the service code looks pretty good, only one small note.

As for the Prometheus code, I'm pretty sure you need to accumulate the counter. Prometheus only mirrors it. So you will need to keep track of the two counters (per key and per location+asn).

cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Show resolved Hide resolved
service/tcp.go Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

It would be helpful to run the benchmarks with and without your changes, then diff, to make sure that's no significant difference we are not accounting for.

Copy link
Author

Choose a reason for hiding this comment

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

panic: duplicate metrics collector registration attempted [recovered]
goos: linux
goarch: amd64
pkg: github.com/Jigsaw-Code/outline-ss-server/internal/integration_test
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
                  │ bench_master.log │          bench_branch.log           │
                  │      sec/op      │    sec/op     vs base               │
TCPThroughput-6         11.51µ ±  2%   11.54µ ±  3%       ~ (p=0.796 n=10)
TCPMultiplexing-6       122.2µ ±  9%   125.2µ ±  8%       ~ (p=0.190 n=10)
UDPEcho-6               135.4µ ± 53%   128.9µ ± 35%       ~ (p=0.853 n=10)
UDPManyKeys-6           1.449m ± 66%   1.113m ± 78%       ~ (p=0.165 n=10)
geomean                 128.9µ         120.0µ        -6.93%

                │ bench_master.log │       bench_branch.log       │
                │       mbps       │    mbps     vs base          │
TCPThroughput-6         694.8 ± 2%   693.0 ± 2%  ~ (p=0.781 n=10)

                  │ bench_master.log │           bench_branch.log           │
                  │       B/op       │     B/op       vs base               │
TCPThroughput-6          11.00 ± 55%     10.00 ± 50%       ~ (p=0.897 n=10)
TCPMultiplexing-6      81.96Ki ±  3%   82.05Ki ±  2%       ~ (p=0.684 n=10)
UDPEcho-6              5.282Ki ±  1%   5.287Ki ±  1%       ~ (p=0.540 n=10)
UDPManyKeys-6          128.3Ki ± 63%   128.3Ki ± 74%       ~ (p=0.382 n=10)
geomean                4.942Ki         4.828Ki        -2.30%

                  │ bench_master.log │           bench_branch.log           │
                  │    allocs/op     │  allocs/op   vs base                 │
TCPThroughput-6        0.000 ±  0%     0.000 ±  0%       ~ (p=1.000 n=10) ¹
TCPMultiplexing-6      646.5 ±  5%     644.0 ±  3%       ~ (p=0.493 n=10)
UDPEcho-6              97.00 ±  0%     97.00 ±  0%       ~ (p=1.000 n=10) ¹
UDPManyKeys-6          909.0 ± 58%     909.0 ± 69%       ~ (p=0.960 n=10)
geomean                            ²                -0.10%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/Jigsaw-Code/outline-ss-server/internal/slicepool
       │ bench_master.log │        bench_branch.log        │
       │      sec/op      │    sec/op     vs base          │
Pool-6       49.55n ± 26%   43.32n ± 43%  ~ (p=0.645 n=10)

       │ bench_master.log │        bench_branch.log        │
       │       B/op       │    B/op     vs base            │
Pool-6         0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

       │ bench_master.log │        bench_branch.log        │
       │    allocs/op     │ allocs/op   vs base            │
Pool-6         0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

pkg: github.com/Jigsaw-Code/outline-ss-server/ipinfo
                    │ bench_master.log │          bench_branch.log           │
                    │      sec/op      │    sec/op     vs base               │
GetIPInfoFromAddr-6       748.1n ± 38%   700.5n ± 29%       ~ (p=0.515 n=10)
NewMMDBIPInfoMap-6        19.87µ ± 36%   19.15µ ± 28%       ~ (p=0.912 n=10)
geomean                   3.856µ         3.663µ        -5.01%

                    │ bench_master.log │           bench_branch.log            │
                    │       B/op       │     B/op      vs base                 │
GetIPInfoFromAddr-6         56.00 ± 0%     56.00 ± 0%       ~ (p=1.000 n=10) ¹
NewMMDBIPInfoMap-6        1.617Ki ± 0%   1.617Ki ± 0%       ~ (p=1.000 n=10) ¹
geomean                     304.5          304.5       +0.00%
¹ all samples are equal

                    │ bench_master.log │          bench_branch.log           │
                    │    allocs/op     │ allocs/op   vs base                 │
GetIPInfoFromAddr-6         4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
NewMMDBIPInfoMap-6          61.00 ± 0%   61.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                     15.62        15.62       +0.00%
¹ all samples are equal

pkg: github.com/Jigsaw-Code/outline-ss-server/service
                       │ bench_master.log │            bench_branch.log             │
                       │      sec/op      │     sec/op       vs base                │
Locking-6                   247.1n ±  40%    190.8n ±   96%        ~ (p=0.684 n=10)
Snapshot-6                  2.927µ ±  18%    2.717µ ±   12%        ~ (p=0.247 n=10)
ReplayCache_Creation-6      24.60µ ±   8%    24.66µ ±    5%        ~ (p=0.912 n=10)
ReplayCache_Max-6           111.2n ±  10%    102.3n ±   17%        ~ (p=0.516 n=10)
ReplayCache_Min-6           294.8n ±  86%    306.0n ±   55%        ~ (p=1.000 n=10)
ReplayCache_Parallel-6      262.8n ±  10%    242.8n ±   14%        ~ (p=0.247 n=10)
ServerSaltGenerator-6       1.903µ ±   5%    1.840µ ±    3%        ~ (p=0.052 n=10)
TCPFindCipherFail-6         932.6µ ± 128%   1039.9µ ±   25%        ~ (p=0.631 n=10)
TCPFindCipherRepeat-6      121.48µ ± 736%    80.13µ ± 1178%        ~ (p=0.315 n=10)
UDPUnpackFail-6             1.063m ±  46%    1.201m ±   27%        ~ (p=0.684 n=10)
UDPUnpackRepeat-6          312.22µ ±  89%    62.81µ ± 1226%        ~ (p=0.971 n=10)
UDPUnpackSharedKey-6        13.29µ ±  25%    13.10µ ±   20%        ~ (p=0.853 n=10)
geomean                     8.517µ           7.036µ          -17.39%

                       │ bench_master.log │             bench_branch.log             │
                       │       B/op       │      B/op       vs base                  │
Locking-6                    8.000 ±   0%     8.000 ±   0%        ~ (p=1.000 n=10) ¹
Snapshot-6                 8.000Ki ±   0%   8.000Ki ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Creation-6     208.0Ki ±   0%   208.0Ki ±   0%        ~ (p=1.000 n=10)
ReplayCache_Max-6            10.00 ±   0%     10.00 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Min-6            95.00 ±   0%     95.00 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Parallel-6       10.00 ±   0%     10.00 ±   0%        ~ (p=1.000 n=10) ¹
ServerSaltGenerator-6        976.0 ±   0%     976.0 ±   0%        ~ (p=1.000 n=10) ¹
TCPFindCipherFail-6        116.9Ki ±   0%   116.9Ki ±   0%        ~ (p=0.724 n=10)
TCPFindCipherRepeat-6      27.37Ki ± 188%   25.64Ki ± 207%        ~ (p=0.353 n=10)
UDPUnpackFail-6            116.7Ki ±   0%   116.7Ki ±   0%        ~ (p=1.000 n=10) ¹
UDPUnpackRepeat-6         19.603Ki ±  82%   4.918Ki ± 805%        ~ (p=1.000 n=10)
UDPUnpackSharedKey-6       1.166Ki ±   0%   1.166Ki ±   0%        ~ (p=1.000 n=10) ¹
geomean                    1.768Ki          1.567Ki         -11.37%
¹ all samples are equal

                       │ bench_master.log │            bench_branch.log             │
                       │    allocs/op     │   allocs/op    vs base                  │
Locking-6                  1.000 ±   0%      1.000 ±   0%        ~ (p=1.000 n=10) ¹
Snapshot-6                 1.000 ±   0%      1.000 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Creation-6     2.000 ±   0%      2.000 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Max-6          0.000 ±   0%      0.000 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Min-6          1.000 ±   0%      1.000 ±   0%        ~ (p=1.000 n=10) ¹
ReplayCache_Parallel-6     0.000 ±   0%      0.000 ±   0%        ~ (p=1.000 n=10) ¹
ServerSaltGenerator-6      10.00 ±   0%      10.00 ±   0%        ~ (p=1.000 n=10) ¹
TCPFindCipherFail-6       1.605k ±   0%     1.606k ±   0%        ~ (p=0.771 n=10)
TCPFindCipherRepeat-6     123.00 ± 579%      99.50 ± 738%        ~ (p=0.403 n=10)
UDPUnpackFail-6           1.602k ±   0%     1.602k ±   0%        ~ (p=1.000 n=10) ¹
UDPUnpackRepeat-6         259.00 ±  86%      56.50 ± 967%        ~ (p=1.000 n=10)
UDPUnpackSharedKey-6       17.00 ±   0%      17.00 ±   0%        ~ (p=1.000 n=10) ¹
geomean                                 ²                  -13.46%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
@fortuna fortuna self-requested a review March 26, 2024 19:19
Copy link

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

This looks great. Just one small adjustment. Also, were you able to diff the benchmarks?

cmd/outline-ss-server/metrics.go Outdated Show resolved Hide resolved
Commit suggestions.

Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
@sbruens sbruens merged commit 53fd283 into master Mar 27, 2024
4 checks passed
@sbruens sbruens deleted the sbruens/ip-key-metrics branch March 27, 2024 17:46
sbruens added a commit that referenced this pull request Mar 28, 2024
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.

2 participants