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 server connection count metric #464

Merged
6 changes: 6 additions & 0 deletions metricsbp/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ type Config struct {
//
// Optional, defaults to false.
RunSysStats bool `yaml:"runSysStats"`

// ReportServerConnectionCount indicates that you want to publish
// a counter for the number of clients connected to the server
//
// Optional, defaults to false.
ReportServerConnectionCount bool `yaml:"reportServerConnectionCount"`
Copy link
Member

Choose a reason for hiding this comment

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

this is defined in the global metricsbp.Config, but it's only implemented in thrift server, not http server, which is kinda misleading.

I think we should remove the one from metricsbp.Config and only provide the one from thriftbp.ServerConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to set the server config from the application yaml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that desireable? Metrics are not usually the domain of configuration

Copy link
Member

Choose a reason for hiding this comment

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

we can also just remove this configuration and just report it always. an additional counter/gauge adds negligible overhead to a go service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if I should update it to always enabled

Copy link
Member

Choose a reason for hiding this comment

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

@kylelemons what do you think to remove this configuration and just always on?

Copy link
Contributor

@kylelemons kylelemons Jan 31, 2022

Choose a reason for hiding this comment

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

The metric is negligible but the additional thunk call per RPC might not be... though it may be dwarfed by thrift overhead.

I don't have enough experience to know, but if both of you agree it's ok to enable by default then it probably is

Copy link
Member

Choose a reason for hiding this comment

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

which is why I prefer to just move it to thriftbp.ServerConfig instead of default on :)

in the current cookiecutter we always need to pass in thriftbp.ServerConfig to thriftbp.NewBaseplateServer manually, so adding a config there manually to make it opt-in is not something too out of the scope.

If you want it from yaml, you can just make thriftbp.ServerConfig part of your Config struct and override the Processor field (as that cannot be deserialized from yaml anyways).

}

// InitFromConfig initializes the global metricsbp.M with the given context and
Expand Down
12 changes: 11 additions & 1 deletion thriftbp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ type ServerConfig struct {
// If not set none of the requests will be sampled.
ReportPayloadSizeMetricsSampleRate float64

// Optional, used only by NewBaseplateServer.
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
//
// Report the number of clients connected to the server.
ReportServerConnectionCount bool
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved

// Optional, used only by NewServer.
// In NewBaseplateServer the address set in bp.Config() will be used instead.
//
Expand All @@ -69,7 +74,7 @@ type ServerConfig struct {
// and protocol to serve the given TProcessor which is wrapped with the
// given ProcessorMiddlewares.
func NewServer(cfg ServerConfig) (*thrift.TSimpleServer, error) {
var transport *thrift.TServerSocket
var transport thrift.TServerTransport
if cfg.Socket == nil {
var err error
transport, err = thrift.NewTServerSocket(cfg.Addr)
Expand All @@ -80,6 +85,10 @@ func NewServer(cfg ServerConfig) (*thrift.TSimpleServer, error) {
transport = cfg.Socket
}

if cfg.ReportServerConnectionCount {
transport = &CountedTServerTransport{transport}
}

server := thrift.NewTSimpleServer4(
thrift.WrapProcessor(cfg.Processor, cfg.Middlewares...),
transport,
Expand Down Expand Up @@ -115,6 +124,7 @@ func NewBaseplateServer(
}).ToThriftLogger()
}
cfg.Addr = bp.GetConfig().Addr
cfg.ReportServerConnectionCount = bp.GetConfig().Metrics.ReportServerConnectionCount
cfg.Socket = nil
srv, err := NewServer(cfg)
if err != nil {
Expand Down
49 changes: 49 additions & 0 deletions thriftbp/server_transport.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package thriftbp

import (
"github.com/apache/thrift/lib/go/thrift"
"github.com/go-kit/kit/metrics"
"github.com/reddit/baseplate.go/metricsbp"
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
)

// TODO(marco.ferrer):1/4/22 Replace with metric name that conforms to bp conventions
const meterNameTransportConnCounter = "thrift.conn.count"
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved

type CountedTServerTransport struct {
thrift.TServerTransport
}

func (m *CountedTServerTransport) Accept() (thrift.TTransport, error) {

marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
transport, err := m.TServerTransport.Accept()
if err != nil {
return nil, err
}

return NewCountedTTransport(transport), nil
}

type CountedTTransport struct {
thrift.TTransport
counter metrics.Counter
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
}

func NewCountedTTransport(transport thrift.TTransport) thrift.TTransport {
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
return &CountedTTransport{
TTransport: transport,
counter: metricsbp.M.Counter(meterNameTransportConnCounter),
}
}

func (m *CountedTTransport) Close() error {
m.counter.Add(-1)
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
return m.TTransport.Close()
}

func (m *CountedTTransport) Open() error {
err := m.TTransport.Open()
if err == nil {
m.counter.Add(1)
}
marcoferrer marked this conversation as resolved.
Show resolved Hide resolved
return err
}