Consider includeLabelInSql to be on by default

[rbygrave]

Recently we added #3362 which when turned on include the "label" in generated SQL select queries. For query beans, this will automatically be the class and method that invoked the query.

Example:

select /* MyService.myMethod */ t0.id, ...

This feature is really there to assist Database side tools that look for expensive queries in total time and slow queries etc.

Now having turned this on myself, I'm liking this feature when running tests and looking at SQL logs and that makes me think that maybe this feature should some day be on by default (and then people could configure to turn it off if they wished).

So I'm keen for people to turn this on themselves and ponder if there are any reasons why it should not be used or if you'd be happy for this feature to become on by default.

Thanks, Rob.

---

Some background:

From an application / ebean perspective we can obtain the query execution metrics (count, mean, max, total) via Database.metaInfo().collectMetrics(). Some applications will be collecting these metrics every minute and publishing them to a performance monitoring tool (like Grafana etc).

The Labels on these metrics for query bean queries are the same labels being included into the SQL as an inline comment.

Some details on Labels:
As a detail, you'll note that sometimes these have line numbers and most often they don't. The reason why we prefer to NOT include the line number is because we want these metric labels to be as stable as we can to compare how they change over time - if we include the line number then these can change frequently as new lines of code are added etc. The line number will be included when it is detected that there are multiple queries for the same type in the same method and at that point we know that the method name itself does not sufficiently identify where the query is executed from so we need to include the line number in the label.

As another detail, you'll note that secondary queries and lazy loading queries append as a path to the label of the "origin" query. For example let's say we have a label like "MyService.myMethod" where a query for orders is executed, we might have a label "MyService.myMethod_customer" (for a secondary query for the customer of an order). "MyService.myMethod_customer_billingAddress" (for a secondary query of the customers billing address). There is a "__lazy" suffix added when a secondary query is a lazy loading query.

[jnehlmeier]

Are inline comments showing up in pg_stat_statements and thus may flood that view with the same query if that query has been called from many locations (which would change the inline comment)?

[rbygrave]

showing up in pg_stat_statements

Yes.

if that query has been called from many locations (which would change the inline comment)?

Yes, the default inline comment (without explicitly setting a label on the query) would reflect the calling location as calling class + method.

may flood that view with the same query if that query has been called from many locations

So I'm pondering how likely this is going to be? I'm not expecting this in the code bases I see so I'm pondering this.

A "workaround" for this is to either (A) turn off the feature or (B) use an explicit label on those queries that we want to be exactly the same [rather than default to the calling class and method] like query.setLabel("CustomerById").setId(id).findOne() ... would this be a PITA?

Do you have a lot of the same query executed from different locations? Is that more the "find by id" query or something else?

What do you think about the workarounds? Would you use the explicit query label or just turn off the feature?

---

The other case where this might be problematic is people are looking at longer term comparisons and where the existing metrics don't have any inline comments ... and then this feature is turned on and now the new queries with the inline comments are hard to match to the old metrics.

For this case, people would turn off this feature I think (or suck it up and reset the metrics because there is value in seeing the reference back to the calling code in the sql itself).

[jnehlmeier]

would reflect the calling location as calling class + method.

So if I have some DAO / Finder like class then this class + method would be used? Or do you skip these too using some clever heuristics (e.g. checking if the call site implements Finder) and walk further up the call hierarchy to end up in business logic?

Do you have a lot of the same query executed from different locations? Is that more the "find by id" query or something else?

Entities can share some common functionality like soft-delete, syncable with other systems, versionable / change tracking, etc.
In that case you likely have some generic DAO / Finder classes containing queries for these kind of functionalities and you might have used these generic DAO / Finder classes as super class of the concrete DAO / Finder for a given entity. So when using getClass() to figure out the call site many DAO / Finder classes might show up for the same query.

Of course this also includes basic stuff like findById, findByIds, findAll, delete, ... in the top most Dao / Finder super class.

Would you use the explicit query label or just turn off the feature?

With 400+ entities/DAOs I would probably turn the feature off immediatly and think about it some time/years later once I have (reserved) time for it.

Generally I like the idea but usually the more complex queries are the ones that are interesting to watch since they can become slow. And because these queries are complex you usually find them in code relatively quickly.

[rob-bygrave]

some DAO / Finder like class then this class + method would be used?

Yes in that it's not an ebean class method. You can see this right now if we look at the name or location attribute of the ebean metrics. For example, when running tests, we could turn on the dumping of metrics on shutdown via:

ebean.dumpMetricsOnShutdown=true
ebean.dumpMetricsOptions=sql,hash,loc

The loc there means "location" so you'll be able to see how that works with the generic code in DAO's.

And because these queries are complex you usually find them in code relatively quickly.

The argument for this is that nearly anyone can find the query quickly in the code base. They do not have to be familiar with the code base being either relatively new to the project or being a SRE (Site Reliability Engineer).

---

To me the unexpected benefit of this is that it really helps when running tests with logged sql - it's just faster and easier to grok which query is which when running tests.

FWIW: My offline discussions with others are very supportive of this being on by default.

[rob-bygrave]

With 400+ entities/DAOs

Just to add my thought that to me the bigger the application the bigger the motivation to turn this on.