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

Add /status endpoint to signer #4280

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Add /status endpoint to signer #4280

merged 2 commits into from
Jan 29, 2024

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jan 23, 2024

I've been documenting the workflow for running the signer, and I think it would be helpful to expose a GET /status endpoint to help setup and monitor the signer. This PR responds with 200 OK for that endpoint.

Note that this could potentially conflict with an event observer path in the future. I'm not sure if the Stacks Blockchain API exposes a similar endpoint on the event receiver, but if so, we should follow use same route. @zone117x does that exist?

I could also imagine having an endpoint similar to /v2/status on the node, where more helpful state is returned, but I'm not sure how much we want to expose the fact that a specific IP is running a specific signer.

@hstove hstove requested a review from jferrant January 23, 2024 16:33
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (5d18590) 83.37% compared to head (28a459a) 83.08%.

Files Patch % Lines
libsigner/src/tests/mod.rs 83.33% 2 Missing ⚠️
stacks-signer/src/runloop.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4280      +/-   ##
==========================================
- Coverage   83.37%   83.08%   -0.29%     
==========================================
  Files         434      434              
  Lines      308750   308770      +20     
==========================================
- Hits       257419   256541     -878     
- Misses      51331    52229     +898     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

libsigner/src/events.rs Outdated Show resolved Hide resolved
@jferrant
Copy link
Collaborator

If you see value in having this, I have no problem with it. It can always be removed later if we change our mind.

@jcnelson jcnelson self-requested a review January 24, 2024 18:54
@@ -211,6 +213,13 @@ impl EventReceiver for SignerEventReceiver {
return Err(EventError::Terminated);
}

if request.url() == "/status" {
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, this is a POST not a GET, right? These are events that are POSTed by the node to the signer's event observer client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this code it would respond to both GET and POST, though I have no problem keeping it to just GET. This is for the signer's event receiver, but the goal is to have a status endpoint for monitoring, which is why GET made sense.

This does "pollute" the event receiver, but the alternative is to have a separate endpoint/port to listen on for monitoring, if we want to have some kind of automated status check.

@hstove hstove force-pushed the feat/signer-status-endpoint branch from 3958461 to c312222 Compare January 26, 2024 17:29
@hstove hstove requested a review from jferrant January 26, 2024 17:30
@hstove hstove force-pushed the feat/signer-status-endpoint branch from c312222 to 28a459a Compare January 29, 2024 19:12
@hstove hstove merged commit bdb2e3c into next Jan 29, 2024
2 checks passed
@hstove hstove deleted the feat/signer-status-endpoint branch January 29, 2024 21:09
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants