-
Notifications
You must be signed in to change notification settings - Fork 73
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
marcoferrer
merged 11 commits into
reddit:master
from
marcoferrer:add-metrics-for-connection-count
Feb 1, 2022
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7352125
add server connection count metric
marcoferrer 16fe632
add error check for open metric
marcoferrer 6b3f16e
update to noop on false
marcoferrer 684b297
update metric name
marcoferrer 585e70f
make transport wrapper private
marcoferrer 317d667
Update thriftbp/server_transport.go
marcoferrer 35b30fd
Merge branch 'master' into add-metrics-for-connection-count
marcoferrer 35261de
Update config.go
marcoferrer 1b0cfad
switch for gauge for connection metrics
marcoferrer ab63b93
remove metrics config flag
marcoferrer da30bbf
update docs
marcoferrer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package thriftbp | ||
|
||
import ( | ||
"sync" | ||
|
||
"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
|
||
) | ||
|
||
const meterNameTransportConnCounter = "thrift.connections" | ||
|
||
type CountedTServerTransport struct { | ||
thrift.TServerTransport | ||
} | ||
|
||
func (m *CountedTServerTransport) Accept() (thrift.TTransport, error) { | ||
transport, err := m.TServerTransport.Accept() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return newCountedTTransport(transport), nil | ||
} | ||
|
||
type countedTTransport struct { | ||
thrift.TTransport | ||
|
||
counter metrics.Counter | ||
closeOnce sync.Once | ||
} | ||
|
||
func newCountedTTransport(transport thrift.TTransport) thrift.TTransport { | ||
return &countedTTransport{ | ||
TTransport: transport, | ||
counter: metricsbp.M.Counter(meterNameTransportConnCounter), | ||
} | ||
} | ||
|
||
func (m *countedTTransport) Close() error { | ||
m.closeOnce.Do(func() { | ||
m.counter.Add(-1) | ||
marcoferrer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
return m.TTransport.Close() | ||
} | ||
|
||
func (m *countedTTransport) Open() error { | ||
if err := m.TTransport.Open(); err != nil { | ||
return err | ||
} | ||
m.counter.Add(1) | ||
return nil | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 fromthriftbp.ServerConfig
?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
tothriftbp.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 yourConfig
struct and override theProcessor
field (as that cannot be deserialized from yaml anyways).