-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make metrics prefix configurable #615
Comments
Adding `metricsPrefix` field to HikariConfig and a default of "pool" because that's what it is currently. This is work towards brettwooldridge#615 Setting the metricsPrefix by default to the poolname + "pool" so that it matches what's going on in the CodahaleHealthChecker and it will be easier to update that and keep the old behavior for other people who don't need to set a custom prefix on there metrics.
Updated metrics related classes to use the metricsPrefix from HikariConfig rather than a combination of the poolname (from config) plus the hard coded word "pool". This gives more control to users who need to set the prefixes of their metrics to something specific such as the person who raised brettwooldridge#615
@jnehlmeier What is an example of an InfluxDB metric name under your scheme? When considering DropWizard metrics there is a naming convention that should be followed:
Presumably you have something pulling metrics from DropWizard and inserting them into InfluxDB. Isn't that the better location to perform your application specific translation? |
@brettwooldridge Of course we have a working solution but its more complex then it needs to just because we can not freely configure the metric prefix used by HikariCP. In addition I think you have to open up metric naming anyways because Metrics 4.0 will not only support String names for metrics but also an instance of class MetricName which supports tags (to avoid overly long metric names as in Metrics 3.x just to encode some data in them). But MetricName is immutable so if people want to add tags to the metrics of HikariCP you would need to add some API for it (see: #624 (comment)) Anyways in our reporting to InfluxDB we generally distinguish two kinds of metrics: single valued (counter, gauge) and multi valued (timer, histogram, meter). We do this because in InfluxDB we want to combined single valued metrics if they belong to the same measurement. Some examples:
Now our reporter parses these strings assuming the form
Now lets look at HikariCP. Because metric names are tied to the pool name we construct a pool name like
Because of this our parsing code is now a bit different then we would like it because we need to ignore "pool" to get our desired result in InfluxDB as described above. IF you ever change the Metric prefix in HikariCP we also might have to adjust our parsing code. Ideally we would like to provide our own metric prefix and ideally the pool name and the metric name should be configureable independently. |
@jnehlmeier Ok, I understand your usecase. I would say that your choice of naming convention somewhat cuts across the grain of the design of HikariCP. Your "jdbc_pool" prefix is essentially a duplication of the "pool" component in the distinguished name used by HikariCP. It also cuts across traditional hierarchical component naming convention. In nearly any "hierarchical" component naming system you find the "left-most" component name is "most distinguishing", followed by the next most, and proceeding to the right. For example:
It would be "strange" to see a component hierarchy like:
The assumption, based on hierarchical naming, is that a pool name of The "pool" component is added as a guard against breaking changes in the future if, for example, we want to introduce different connection-based metrics under a "connection" component. |
@brettwooldridge I have just seen that Hikari also allows setting a MetricsTrackerFactory. Never realized it or it wasn't available in the Hikari version we used back in the days. Looks like we could now just implement our own DropwizardMetricsTracker and choose whatever metric name we want? If thats true and you stick to this approach for a future "connection" component then I guess this issue can be closed. |
Currently the pool name is used as metrics prefix with an added "pool" token.
We use InfluxDB to store & query metrics data and InfluxDB has the concept of tagging metric values so you can quickly query them using a SQL-like WHERE clause. Because metrics can only have a name these tags are computed from the metric name and thus our metrics have a pretty strict form:
<measurement>.<tag-key>.<tag-value>......<field>
. In addition to tags from metric names there might be global tags like the webapp's context path to distinguish metrics per webapp.Currently we need to workaround that HikariCP appends a "pool" token to the pool name when constructing metrics. Also we would like to add some information to the pool name that should not result in a tag for metrics.
We could make our metric name parsing algorithm more complex but I think it would be better if you would decouple the the metric name from the pool name and let developers configure both at their will (the default can be as it is now). So ideally HikariCP should create metrics using something like
MetricRegistry.name(hikariConfig.getPoolMetricsPrefix(), "Wait")
The text was updated successfully, but these errors were encountered: