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(cli): add metrics server to iroh doctor #2292

Merged
merged 2 commits into from
May 22, 2024
Merged

feat(cli): add metrics server to iroh doctor #2292

merged 2 commits into from
May 22, 2024

Conversation

Arqu
Copy link
Collaborator

@Arqu Arqu commented May 15, 2024

Description

Folks try to look at metrics while doctor alone is running (note this clashes by default with the iroh node if both are running locally on port :9090). Also kind of inconvenient given if you want to look at a running iroh node when doctor is running.

Suggestions welcome, maybe default off metrics on doctor?

Also most metrics are not really used in the doctor path, but we can fix that as we figure out what's cool to measure.

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@Arqu Arqu requested a review from dignifiedquire May 15, 2024 10:04
@Arqu Arqu self-assigned this May 15, 2024
@Arqu Arqu added this to the v0.17.0 milestone May 15, 2024
@divagant-martian
Copy link
Contributor

divagant-martian commented May 15, 2024

Maybe I'm doing something wrong but it seems to me this conflicts with plot. If a node is running with metrics, then I get a tui dash but I get an error as well, and if no iroh node is running I get no error but no data (which I guess is expected)

Conversely, if I start doctor plot first I get an empty dash but now I cannot start a node with metrics for plot to track

@eminence
Copy link

I think my expectations are:

  • If nothing is running (no iroh node, no doctor), and I run doctor plot, I would not expect the metrics server to be started (because the plotting tool isn't doing any magic socket stuff)
  • If no iroh node is running, but I'm doing testing with doctor accept or doctor connect, I'd like to be able to use doctor plot to monitor things
  • If an iroh node is running, and I run doctor accept, I'm not 100% sure what I expect. Part of me thinks that maybe the doctor should use the running node for its operations, but for now let's assume there are good reasons to not do that. So I think I'd like the chance to plot both the iroh node and the doctor.

So maybe something like:

  • doctor accept and doctor connect gain a new --expose-metrics option, which is off-by-default
  • The --expose-metrics option can take an optional port number to listen on, instead of 9090
  • The doctor plot command gains an option to specify the port/URL to connect to
  • If I have both a node running and a doctor running, it's my responsibility to manage the ports correctly to avoid any collision

@dignifiedquire dignifiedquire added this pull request to the merge queue May 22, 2024
@dignifiedquire dignifiedquire removed this pull request from the merge queue due to a manual request May 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 22, 2024
## Description

Folks try to look at metrics while doctor alone is running (note this
clashes by default with the iroh node if both are running locally on
port :9090). Also kind of inconvenient given if you want to look at a
running iroh node when doctor is running.

Suggestions welcome, maybe default off metrics on `doctor`?

Also most metrics are not really used in the `doctor` path, but we can
fix that as we figure out what's cool to measure.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] Documentation updates if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
@Arqu Arqu force-pushed the arqu/doc_metrics branch from 00b57a0 to a17d924 Compare May 22, 2024 20:23
@Arqu Arqu force-pushed the arqu/doc_metrics branch from a17d924 to b125d08 Compare May 22, 2024 20:24
@Arqu
Copy link
Collaborator Author

Arqu commented May 22, 2024

This should now work decently nice for the above use cases. Metrics are off by default and you can turn them on when you like by setting the metrics port.
eg

shell A
iroh --metrics-port 9091 doctor relay-urls
shell B
iroh doctor plot --scrape-url http://localhost:9091 iroh_requests_total_total

@Arqu Arqu added this pull request to the merge queue May 22, 2024
Merged via the queue into main with commit d635d93 May 22, 2024
23 checks passed
@Arqu Arqu deleted the arqu/doc_metrics branch May 22, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants