Add support for per-query metrics in Hibernate statistics(#1006) #1461

pawel2401 · 2019-06-18T23:13:00Z

Resolves #1006

micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jpa/HibernateMetrics.java

pawel2401 · 2019-09-11T19:31:13Z

@shakuzen are you fine with the changes? Should I change smth else?

...meter-core/src/main/java/io/micrometer/core/instrument/binder/jpa/HibernateQueryMetrics.java

pawel2401 · 2019-11-18T21:48:15Z

@shakuzen Anything else do you see that would be good to change?

shakuzen · 2020-03-19T08:08:44Z

...meter-core/src/main/java/io/micrometer/core/instrument/binder/jpa/HibernateQueryMetrics.java

+                QueryStatistics queryStatistics = statistics.getQueryStatistics(query);
+                if (Objects.nonNull(queryStatistics)) {
+                    String queryName = meterRegistry.config().namingConvention().tagValue(query);
+                    if (Objects.isNull(Search.in(meterRegistry).tags("query", queryName).functionCounter())) {


Is the point of this check because there could be multiple of the same query returned by statistics.getQueries()?

Emm If I remember correctly there should not, it is a little bit redundant check

…-metrics#1006)

…breaking the meter name

#Remove hibernate.query.executions.avg as is redundant

shakuzen

Sorry for the long delay on getting this merged, and thank you for making the changes requested during review. Great work.

shakuzen · 2020-03-19T10:37:43Z

...meter-core/src/main/java/io/micrometer/core/instrument/binder/jpa/HibernateQueryMetrics.java

+
+/**
+ * A {@link MeterBinder} implementation that provides Hibernate query metrics. It exposes the
+ * same statistics as would be exposed when calling {@link Statistics#getQueryStatistics(String)}.


We should have a warning about potential high cardinality depending on the variety of queries one's application executes. I can add this as a polish commit post merge.

shakuzen · 2020-03-19T10:40:53Z

...meter-core/src/main/java/io/micrometer/core/instrument/binder/jpa/HibernateQueryMetrics.java

+
+    /**
+     * Register hibernate loadEvent listener - event trigger the metrics registration,
+     * only select queries are recorded in QueryStatistics {@link org.hibernate.stat.spi.StatisticsImplementor}


If we think it would be helpful to users to have this information, we should add it to one of the public methods or the class JavaDoc. On a private method, it will not be noticed by most users.

shakuzen · 2020-03-19T10:42:56Z

...r-core/src/test/java/io/micrometer/core/instrument/binder/jpa/HibernateQueryMetricsTest.java

+    }
+
+    private HibernateQueryMetrics.MetricsEventHandler createMetricsEventHandlerMock() {
+        HibernateQueryMetrics hibernateQueryMetricsMock = Mockito.mock(HibernateQueryMetrics.class);


It feels like we're mocking too much when we are even mocking the class under test. I will try to rework the tests a bit to get something that feels closer to real usage. We could even make tests that use an in-memory database and normal Hibernate with that. This can be done post-merge, though.

After looking at this more closely, I see we're essentially testing the MetricsEventHandler in isolation. That's fine for this. I'll open an issue to add tests with not-mocked Hibernate classes.

checketts reviewed Jun 19, 2019

View reviewed changes