Skip to content
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

quarkus.hibernate-orm.statistics can not be set to false if metrics.enabled is true #20666

Closed
antonwiens opened this issue Oct 11, 2021 · 13 comments · Fixed by #20693
Closed

Comments

@antonwiens
Copy link

The description of quarkus.hibernate-orm.statistics is not reflecting the reality.

The description here:

* Whether statistics collection is enabled. If 'metrics.enabled' is true, then the default here is

Does not match the boolean condition:

|| (hibernateOrmConfig.statistics.isPresent() && hibernateOrmConfig.statistics.get())) {

If hibernateOrmConfig.metricsEnabled is true, then the statistics property is completley ignored.
Therefor setting statistics to false is not disabling statistics.

I think this is wrong and should be changed to be allowed to disable statistics logging even if metrics are enabled and to match the description. This is polluting logs in production environments.

This affects the current master and Quarkus 2.2.3.Final

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2021

/cc @Sanne, @ebullient, @gsmet, @jmartisk, @yrodiere

@Sanne
Copy link
Member

Sanne commented Oct 11, 2021

What do you mean by "polluting logs", could you clarify that?

Thanks for the report :)

@antonwiens
Copy link
Author

What do you mean by "polluting logs", could you clarify that?

Thanks for the report :)

It just means that i don't need the log statement, can't disable it and its spamming the log server :).

I will now use the "internal" log filter to remove it.

@jmartisk
Copy link
Contributor

quarkus.hibernate-orm.statistics controls whether statistics are collected or not. To be able to do metrics, you need statistics, otherwise you can't turn expose them to the outside as metrics. So, my take from this issue is that you want to be able to specifically disable statistics "logging", rather than disable statistics altogether, right?

@antonwiens
Copy link
Author

antonwiens commented Oct 12, 2021

quarkus.hibernate-orm.statistics controls whether statistics are collected or not. To be able to do metrics, you need statistics, otherwise you can't turn expose them to the outside as metrics. So, my take from this issue is that you want to be able to specifically disable statistics "logging", rather than disable statistics altogether, right?

For my case, this is correct. But is this switch meaningful otherwise? I mean, if you dont want metrics, what is the point of setting this to true if you don't want the log statement? Does it do anything else than generate the log statements (if you don't want the metrics)?

@jmartisk
Copy link
Contributor

Collecting Hibernate statistics is useful even without exposing them as metrics, if you want to inspect the statistics programmatically. You can obtain the Statistics object using entityManager.getEntityManagerFactory().unwrap(SessionFactory.class).getStatistics() and read the numbers from it.

@antonwiens
Copy link
Author

Collecting Hibernate statistics is useful even without exposing them as metrics, if you want to inspect the statistics programmatically. You can obtain the Statistics object using entityManager.getEntityManagerFactory().unwrap(SessionFactory.class).getStatistics() and read the numbers from it.

Thanks, i didn't know that.

Will you consider adding a switch to disable the statistics log statement?

@Sanne
Copy link
Member

Sanne commented Oct 12, 2021

Will you consider adding a switch to disable the statistics log statement?

Yes, absolutely. Enabling statistics shouldn't imply that it will also log them. And rather than a switch I think we should just disable this but I need to check why this was enabled.

@Sanne
Copy link
Member

Sanne commented Oct 12, 2021

Ok I don't think it was intentionally enabled - the option hibernate.session.events.log controls this, but its default value is enabled when statistics are enabled.

I don't think that's a good idea, I'll disable it by default.

@jmartisk
Copy link
Contributor

I don't think that's a good idea, I'll disable it by default.

+1. And add a quarkus. property that enables it

@Sanne
Copy link
Member

Sanne commented Oct 12, 2021

I don't think that's a good idea, I'll disable it by default.

+1. And add a quarkus. property that enables it

I actually don't think this is very useful so let's wait for someone to ask for it before adding more config properties :)

@jmartisk
Copy link
Contributor

In that case I think the same change (turn off stats logging by default) should be done in pure Hibernate too :)
It feels weird that pure Hibernate logs them by default, but Quarkus won't support that at all.

@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Oct 12, 2021
@Sanne
Copy link
Member

Sanne commented Oct 12, 2021

@jmartisk agreed; I'll propose it there - we are less flexible having strict backwards compatibility rules though so this might need to wait a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants