Skip to content

Add prometheus - #5

Open
Brindrajsinh-Chauhan wants to merge 6 commits into
circlefin:mainfrom
kaleido-io:add-prometheus
Open

Add prometheus#5
Brindrajsinh-Chauhan wants to merge 6 commits into
circlefin:mainfrom
kaleido-io:add-prometheus

Conversation

@Brindrajsinh-Chauhan

Copy link
Copy Markdown

This PR adds Prometheus metrics to the remote signer to have monitoring of this components

Metrics include

  • Process metrics
  • Golang metrics
  • And some custom metrics like
# HELP arc_remote_signer_request_total Total number of signing requests
# TYPE arc_remote_signer_request_total counter
arc_remote_signer_request_total{type="publickey"} 1
# HELP arc_remote_signer_request_total_success Total number of successful signing requests
# TYPE arc_remote_signer_request_total_success counter
arc_remote_signer_request_total_success{type="publickey"} 1

fn()
}
log.Printf("initiating graceful shutdown of http server at %s", s.listener.Addr().String())
if s.listener != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Normal shutdown can terminate the process here.

I reproduced this against f19a29a with a focused package test: Run() starts Serve() in a goroutine, then Shutdown() manually closes s.listener before calling s.server.Shutdown(...). That makes Serve() return:

accept tcp 127.0.0.1:...: use of closed network connection

That error is not http.ErrServerClosed, so the goroutine reaches log.Fatalf("failed to serve: %v", err) and exits the process during a normal lifecycle shutdown.

The same path also ignores the caller-provided shutdown context by calling s.server.Shutdown(context.Background()).

Minimal fix is to let http.Server.Shutdown(ctx) close the listener and honor the provided context:

func (s *RunnableImpl) Shutdown(ctx context.Context) error {
	for _, fn := range s.beforeShutdownFns {
		fn()
	}

	log.Printf("initiating graceful shutdown of http server at %s", s.listener.Addr().String())
	if err := s.server.Shutdown(ctx); err != nil {
		return err
	}
	log.Printf("http server gracefully stopped")
	return nil
}

Focused verification:

  • current PR code fails with failed to serve: accept tcp ...: use of closed network connection
  • the change above passes go test ./internal/common/http/server

go test ./... is currently blocked by missing generated proto/pb, so I scoped verification to the affected HTTP package.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I have pushed another commit to address the comment. Would be grateful if you could review it again.

Thanks again

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.

2 participants