Skip to content

Conversation

mdeinum
Copy link
Contributor

@mdeinum mdeinum commented Jul 24, 2015

When hibernate is on the classpath and an EntityManagerFactory is configured then
register a HibernateMetrics bean so that the statistics from hibernate get published
to the metrics endpoint.

The exposed statistics are the same as those that would be exposed using Statistics.logSummary
from hibernate itself.

Issue: #2157

@mdeinum
Copy link
Contributor Author

mdeinum commented Aug 24, 2015

Any thoughts on adding this to Spring Boot (or the quality of the request :) )

@snicoll
Copy link
Member

snicoll commented Aug 24, 2015

I've been thinking of that one lately. Have you tried with Hibernate 5? This also exposes only the stats if one primary EntityManager is present. Might be a problem for those who are running with multiple persistence units.

@mdeinum
Copy link
Contributor Author

mdeinum commented Aug 24, 2015

This has been tested with Hibernate 4.3.10 not Hibernate 5 .

I'll take a look at on how to refactor it for multiple persistence units, shouldn't be that difficult (I'll take a peek at the DataSourcePublicMetrics which does the same if I recall correctly).

I have been thinking of making it also a bit more generic so that support for EclipseLink (for instance) could be added. Just like the metrics support for DataSources which adapts per type.

@mdeinum
Copy link
Contributor Author

mdeinum commented Aug 27, 2015

Any thoughts on the updated code?

@snicoll
Copy link
Member

snicoll commented Aug 27, 2015

I am afraid I am personally out of cycles to look at this at the moment. The fact I have no indication it works with Hibernate 5 is also a reason.

@mdeinum
Copy link
Contributor Author

mdeinum commented Sep 10, 2015

I can run the tests against both Hibernate 4.3 and Hibernate 5. Is there an easy way to do that with the current build? Override the version for hibernate?

@snicoll
Copy link
Member

snicoll commented Sep 10, 2015

A validation that it does work with Hibernate 5 is already good on its own. I haven't figured out yet how we'll integrate integration testing for Hibernate 5. Probably a dedicated sample (see #2763)

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 24, 2016
@snicoll
Copy link
Member

snicoll commented Apr 24, 2016

@mdeinum can you rebase your PR and make sure it works still? Thanks!

@mdeinum
Copy link
Contributor Author

mdeinum commented Apr 24, 2016

@snicoll Rebased and fixed some of the new check style rules (tabs instead of spaces, copyright header etc.). Tests still green after this.

@mdeinum
Copy link
Contributor Author

mdeinum commented Apr 24, 2016

@snicoll I did some polishing as there where still some check style errors.

@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Jul 20, 2016
@snicoll snicoll added this to the 1.5.0 M1 milestone Jul 20, 2016
@philwebb philwebb modified the milestones: 1.5.0 M1, 1.5.0, 2.0.0 Aug 30, 2016
@Bert-R
Copy link

Bert-R commented May 16, 2017

Is there any chance this will board Spring Boot?

@snicoll
Copy link
Member

snicoll commented May 17, 2017

@Bert-R there is a milestone version attached to it and I intend to honour it. So, yes. Maybe not in its current form though. There are a lot of stats there.

@Bert-R
Copy link

Bert-R commented May 17, 2017

@snicoll Great, thanks a lot!

@mdeinum
Copy link
Contributor Author

mdeinum commented May 17, 2017

@snicoll anything I can do to polish this up? Good start would probably be to make it work with boot 2.0.0 (maybe we can have a chat about it during SpringIO (?) )

@snicoll
Copy link
Member

snicoll commented May 17, 2017

Of course!

@mdeinum
Copy link
Contributor Author

mdeinum commented May 17, 2017

Ok. My rebase skills are seriously lacking today... I guess we can consider this pull request dead... I'll create a new one.

@philwebb
Copy link
Member

@mdeinum You can force push, no need to close it

@mdeinum
Copy link
Contributor Author

mdeinum commented May 17, 2017

That is what I figured, after the fact. Squashed the commits and did a force push. That cleaned up things...

When hibernate is on the classpath and one or more `EntityManagerFactory`
are configured then register a `HibernatePublicMetrics` bean so that the
statistics from hibernate get published to the metrics endpoint.

The exposed statistics are the same as those that would be exposed
using `Statistics.logSummary`from hibernate itself.
@@ -138,6 +138,11 @@
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-entitymanager</artifactId>
Copy link
Contributor

@rajadilipkolli rajadilipkolli Jun 6, 2017

Choose a reason for hiding this comment

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

since we are intending to include in spring boot 2.0 and spring boot 2.0 supports Hibernate 5.2+ , Since 5.2 entitymanager module is integrated with core module. this dependency is not necessary

Copy link
Member

Choose a reason for hiding this comment

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

@rajadileepkolli that PR hasn't been reviewed in the target branch yet. We'll take care of that at that time.

@snicoll
Copy link
Member

snicoll commented Oct 3, 2017

With the move to micrometer, I am afraid this PR can't be merged. I'd advise to discuss with the micrometer team to see if how this can be added.

@snicoll snicoll closed this Oct 3, 2017
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed priority: normal type: enhancement A general enhancement labels Oct 3, 2017
@snicoll snicoll removed this from the 2.0.0 milestone Oct 3, 2017
@mdeinum mdeinum deleted the gh-2157 branch April 29, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants