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

Closed

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 requested a review from fortuna August 26, 2024 16:17
@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
@github-actions github-actions bot added size/M and removed size/S labels Sep 16, 2024
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
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_service.ts Show resolved Hide resolved
src/shadowbox/server/manager_metrics.spec.ts Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
@daniellacosse daniellacosse requested a review from fortuna October 17, 2024 13:31
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.

Looks good. Just some minor tweaks. And we need to update the YAML file.

src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
src/shadowbox/server/manager_metrics.ts Outdated Show resolved Hide resolved
@daniellacosse daniellacosse requested a review from fortuna October 25, 2024 19:00
@@ -418,6 +418,35 @@ paths:
examples:
'0':
value: '{"bytesTransferredByUserId":{"1":1008040941,"2":5958113497,"3":752221577}}'
/metrics/tunnel-time-location
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be /metrics/tunnel-time? I thought you mention there was another one per access key, but I don't see it. How are we going to report by key?

Did Sander have an opinion on this?

Copy link
Contributor Author

@daniellacosse daniellacosse Oct 30, 2024

Choose a reason for hiding this comment

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

You can see both the metrics at http://127.0.0.1:9092/metrics here:

Screenshot 2024-10-30 at 9 14 21 AM

I think we should make a separate endpoint for the tunnel time report by key

Also I think Sander approved this back when the endpoint was similar, but I didn't check

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will know better about the API once we start using it for real. So perhaps we should pause this and focus on the manager side to better understand the usage.

For instance, we need tunnel time per access key too. Will the manager be forced to issue 3 queries?
Perhaps it will be best to issue one query, maybe under /metrics/usage, that returns all the usage metrics we need. That will simplify the Manager. It will also let us deprecate the bad transfer endpoint. In the future we can just add more to the usage endpoint, instead of keeping adding new endpoints per metric.

@daniellacosse daniellacosse requested a review from fortuna October 30, 2024 13:16
@daniellacosse
Copy link
Contributor Author

Closing in favor of the unified metrics endpoint

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