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

soroban-rpc: Add /metrics endpoint #579

Merged
merged 8 commits into from
Apr 13, 2023
Merged

soroban-rpc: Add /metrics endpoint #579

merged 8 commits into from
Apr 13, 2023

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Apr 12, 2023

What

Add /metrics prometheus endpoint with the following set of metrics:

  • DB (similar to horizon)

    • connections
      • open
      • in-use
      • waited-for
      • wait-duration
    • queries per-method
      • mean duration
      • count
  • General

    • Version running
    • Uptime (comes out of the box w prometheus, I think)
    • Golang stuff like goroutines etc
  • Logging

    • Lines/entries logged (with severity)

Why

#496

This PR does not provide all the metrics specified in the issue above. The remaining metrics be included in separate upcoming PRs.

Known limitations

[N/A]

@tamirms tamirms requested a review from a team April 12, 2023 09:01
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Tamir, I don't believe that the PrometheusRegistry should be part of the local configuration.
( unless you can convince me otherwise ).
I think that it should be a member of the daemon.

The local config is for entries that we would load from a configuration file. I don't believe the registry is part of that..

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Could you please add a unit test to confirm that this PR work ?

@tamirms
Copy link
Contributor Author

tamirms commented Apr 12, 2023

@tsachiherman can you take another look? I think I've addressed your comments

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Tamir, my intent in asking for a unit test was to have a http-client unit test that would verify that the metrics endpoint works as expected.

I don't have any reason to believe that it wouldn't, and I don't want to block you from merging this one in - but I do want to have ( eventually ) a concrete unit test to cover all the http endpoints.

@tamirms
Copy link
Contributor Author

tamirms commented Apr 13, 2023

@tsachiherman could you take another look? I believe I've addressed your feedback.

@tamirms tamirms requested a review from tsachiherman April 13, 2023 16:42
@tamirms tamirms enabled auto-merge (squash) April 13, 2023 17:49
@tamirms tamirms merged commit 8897b45 into main Apr 13, 2023
@tamirms tamirms deleted the metrics branch April 13, 2023 17:58
@tsachiherman tsachiherman linked an issue Apr 21, 2023 that may be closed by this pull request
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.

soroban-rpc: Add /metrics endpoint
4 participants