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(server): add getTunneltime to manager metrics #1581

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

daniellacosse
Copy link
Contributor

@daniellacosse daniellacosse commented Aug 23, 2024

successful result from digital ocean droplet:

Screenshot 2024-09-11 at 3 38 22 PM

TODOs:

  • figure out how to actually send traffic through the endpoint locally
  • set up server in digital ocean and test it that way
  • write test

Copy link
Collaborator

@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 for the PR. This is exactly the kind of discussion I was looking for!

src/shadowbox/model/metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/model/metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/model/metrics.ts Outdated Show resolved Hide resolved
@daniellacosse daniellacosse added the needs test The PR needs more tests label Aug 26, 2024
@github-actions github-actions bot added size/S and removed size/XS labels Aug 27, 2024
@daniellacosse daniellacosse force-pushed the daniellacosse/tunneltime/add_get_tunneltime branch from 40975be to cf4383b Compare September 5, 2024 13:19
@github-actions github-actions bot added size/XS and removed size/S labels Sep 5, 2024
@github-actions github-actions bot added size/S and removed size/XS labels Sep 11, 2024
@daniellacosse daniellacosse marked this pull request as ready for review September 11, 2024 19:54
src/shadowbox/server/manager_metrics.ts Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_service.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_service.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_service.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/M and removed size/S labels Sep 19, 2024
@@ -40,4 +58,21 @@ export class PrometheusManagerMetrics implements ManagerMetrics {
}
return {bytesTransferredByUserId: usage};
}

async getTunnelTimeByLocation({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer by location. It's broken down by all dimensions. Please rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metric name is shadowsocks_tunnel_time_seconds_per_location as opposed to the shadowsocks_tunnel_time_seconds metric - does that matter to you?

export type TunnelTimeDimension = 'access_key' | 'country' | 'asn';

interface TunnelTimeRequest {
time_window: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's define a Duration type and reuse

Copy link
Contributor Author

@daniellacosse daniellacosse Sep 25, 2024

Choose a reason for hiding this comment

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

You mean like...

interface Duration {
  hours?: number;
  seconds?: number;
}

What happens if both hours and seconds are there?

src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
@@ -599,6 +601,23 @@ export class ShadowsocksManagerService {
}
}

async getTunnelTimeByLocation(req: RequestType, res: ResponseType, next: restify.Next) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename

src/shadowbox/server/manager_metrics.spec.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test The PR needs more tests size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants