add server connection count metric #464
Conversation
marcoferrer
requested review from
fishy,
kylelemons and
konradreiche
and removed request for
a team
metricsbp/config.go
Outdated
| // a counter for the number of clients connected to the server | ||
| // | ||
| // Optional, defaults to false. | ||
| ReportServerConnectionCount bool `yaml:"reportServerConnectionCount"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is there a way to set the server config from the application yaml?
There was a problem hiding this comment.
Is that desireable? Metrics are not usually the domain of configuration
There was a problem hiding this comment.
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.
Let me know if I should update it to always enabled
There was a problem hiding this comment.
@kylelemons what do you think to remove this configuration and just always on?
There was a problem hiding this comment.
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.
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).
Co-authored-by: Kyle Lemons <kyle@kylelemons.net>
metricsbp/config.go
Outdated
| // a counter for the number of clients connected to the server | ||
| // | ||
| // Optional, defaults to false. | ||
| ReportServerConnectionCount bool `yaml:"reportServerConnectionCount"` |
There was a problem hiding this comment.
Is that desireable? Metrics are not usually the domain of configuration
There was a problem hiding this comment.
Besides the configuration field, the code looks good to me.
Regarding the configuration field, I'm fine with either remove it from metricsbp.Config and only provide it in thriftbp.ServerConfig, or just remove it altogether and make it always on. But the current state is misleading and should be avoided.
Outside the scope of this PR, but we should probably come up with a preferred convention for sourcing configuration values from the application yaml. It seems like whenever theres a new option exposed to baseplate users they end up having to hard code their settigns or go through the work of creating a new field in their configuration struct. In this scenario, the path of least resistance usually wins. ie. hardcode settings. |
marcoferrer
force-pushed
the
add-metrics-for-connection-count
branch
from
75166fa
to
ab63b93
Compare
This includes the following changes:
TServerTransportwhich wraps the result ofAcceptwith aMonitoredTTransportTTransportwhich emits counter metrics for invocations ofOpenandClosemetricsbp.Configflag to allow users to enable recording connection count metrics from their application yamlthriftbp.ServerConfigflag to check if reporting connection counts is enabled inside ofNewBaseplateServer