granularity method in QueryMetrics.

[akashdw]

PR to emit granularity dimension for timeseries, search, groupBy, select and topN queries.

[leventov]

Make this method similar to QueryMetrics.interval() and QueryMetrics.duration():

• Move it's declaration(s) next to those methods
• Call it next to the calls of those methods
• Accept QueryType and return void
• No default implementation in QueryMetrics interface itself. It's the point of QueryMetrics to not have default methods - see it's Javadoc.

Also, Granularity.toString() should be more readable than JSON form

[akashdw]

Thanks @leventov, added Select/SearchQueryMetricsFactory with default implementations.

[leventov]

1. Why haven't you added granularity() to root QueryMetrics/DefaultQueryMetrics? To avoid duplicating it 5 times, and also not needing to introduce SearchQueryMetrics and SelectQueryMetrics.

2. Methods added to QueryMetrics after the introduction of this abstraction should have empty default implementations, that emit nothing. See #3954 description, "Evolution of metrics" section. An example of this later addition is #4284, note how it adds empty default implementations: https://github.com/druid-io/druid/pull/4284/files#diff-25e10cfecf5e9bf9ba50838f8b7cd1caR177. To actually emit granularity in your case, you should also create your private implementation of corresponding QueryMetrics in your private extensions and configure it, see #4336.

[akashdw]

Thanks @leventov for reviewing the PR. I totally agree with you on giving more flexibility to users to select which dimensions they want instead druid core deciding it, but IMO granularity is an important dimension and druid core should emit it by default.

I added specialized Select/SearchQueryMetricsFactory because of two reasons:

1. Having specialized factory allows users to add query specific dimensions easily, this could also be done as you suggested to have empty default implementations, that emit nothing. approach.

2. As you suggested in the first comment to have granularity() method in QueryMetrics interface which accepts QueryType and return void is tricky for all Query types b/c getGranularity() method is not available in Query interface and not all Query types support granularity (i.e SegmentMetadataQuery, TimeBoundaryQuery and DataSourceMetadataQuery). I had an offline discussion with @himanshug and we discussed below approaches to emit granularity dimension.

a) add getGranularity() method in Query interface with some default.
b) granularity() method in DefaultQueryMetrics but implementation will require a switch case to
get Query.getType() and then type cast Query object to corresponding QueryType to get
granularity. IMO this approach might not be suitable b/c it requires switch + typecasting.
c) Have query specific MetricsFactory.

Current patch is using option c.

[leventov]

How importance of a dimension is measured? Nobody needed and nobody asked for it yet (I might be wrong about this - please point if), and you are the first one who needs it and probably will stay the only one for some time. So why add extra stuff to everyone's else metrics?

Also if you will no longer need this dimension, with empty default implementation it's safe to remove this method - people who didn't use it won't notice, and people who used to override it in their custom QueryMetrics (to actually emit this dimension -- they have to override!) will be "notified" by API breakage and will have a chance to react - e. g. emit it differently, or raise an issue on Github. If the default implementation emits this dimension, somebody may depend on this fact in non-programmatic way (e. g. parsing) and it's much more dangerous to revoke a dimension in this case. This is also a reason why QueryMetrics interface is introduced, see #3954. And this is why dimension that used to be emitted at the moment of QueryMetrics introduction are still emitted by default.

So please leave the default implementation empty, and actually emit it in your private extension.

[leventov]

On Select/Search, I'm ok either way, but just want to point out that SegmentMetadataQuery and DataSourceMetadataQuery already emit pretty non-sense "intervals" (see their getIntervals()) and it's used in QueryMetrics.interval()

[niketh]

@leventov It doesn't make sense to extend DefaultQueryMetrics. Either we decide to include inside the DefaultQueryMetrics or have it as an extension which is internal (private).

[niketh]

@leventov I didn't read your last comment #4570 (comment). I think we are saying the same thing 👍

[leventov]

@niketh the point of adding methods to QueryMetrics is to call them from core, so that extensions shouldn't touch the core.

[akashdw]

Thanks @leventov. I will also ask other Druid members to share their thought on Granularity dimension b/c its an important metric for our use-case but might not be true for everyone.
I also realized some of the dimension e.g dimensionCardinality, addProcessedRows are not emitted by default and could be emitted using custom extensions. My proposal is to control what to include/exclude via config rather than creating custom extensions.

Config based approach could be:

1. Emit everything by default and a have blacklist config to determine dimensions to exclude.
2. Emit legacy metrics/dims by default and have a allowedList config to determine new metric/dimensions to include. Release a list of new dimensions/metrics in the release note so that people can include new query metrics if they want to.

Config based approach has some advantages:

1. Users don't have to develop/maintain custom extensions to emit metrics but they can still control what metrics they want.
2. Adding/removing query metrics will be easier.

@gianm @himanshug @cheddar @leventov Suggestions ?

[leventov]

@akashdw I considered "config" approach from the very beginning instead of QueryMetrics. In fact it was already questioned in the original PR, see #3954 (comment), And I chosen QueryMetrics (call it "interface" approach) because "config" approach is not agile enough:

• with "config" it's not possible to support the variety dimension and metric forms you may want to emit, e. g. different time resolution for time metrics (default now is milliseconds, and we in fact switched to nanoseconds).
• you cannot skip computations, that are needed to produce some metrics and/or dimensions, that might be significant. See QueryMetrics: abstraction layer of query metrics emitting (part of #3798) #3954 (comment), QueryMetrics.makeBitmapResultFactory()
• Dimensions and metrics are not independent, some may want to emit different set to dimensions for different metrics -- would be very cumbersome to setup a system allowing to config all possible combinations via properties.

So the complexity of the configs needed is very high, it's much harder to refactor them than code, while this system is still fundamentally much more restricted than "interface" approach.

Users don't have to develop/maintain custom extensions to emit metrics but they can still control what metrics they want.

I understand that you don't want to do this but it's just what you need to do, if you want custom metrics and/or dimensions. It was clear when QueryMetrics was added and it's the foundation of it's mechanism, that I mentioned in this thread already: notification about changes via API change => your extension build breakage.

Adding/removing query metrics will be easier.

How this statement is supported?

1. (config) Add a configuration, add documentation for it, add parsing of this configuration, add if-else or other ad-hoc logic to check this configuration throughout the code.

2. (interface) Add a method to QueryMetrics, add empty implementation in DefaultQueryMetrics, use the new method in already set up locations with a QueryMetrics object in hand (the entire point of Refactor QueryRunner to accept QueryPlus: Query + QueryMetrics (part of #3798) #4184 and successors)

Why the first is easier?

[cheddar]

I wonder if we should change the direction of the conversation. One sad thing that we ran into and only just found out is that we've always wanted to have metrics emitted that represent the number of rows scanned. In looking into this, it looks like that has actually been implemented because there is a metric method added for that, but the actual code implementation isn't anywhere to be seen so we're not able to take advantage of the work someone else has already done.

So, maybe we need to discuss when we think a metric or dimension should be in the default, core set and when it shouldn't. If we have that conversation, then the question becomes whether or not "granularity" should be a dimension included on all query metrics that have a granularity.

I tend to think that including granularity isn't a big deal (you will get a lot more cardinality from the id or the segment ids) and it provides a nice level of drill down that can help when trying to understand the performance profile. What are your concerns with adding it to the core set of dimensions?

[leventov]

@cheddar what do you think about adding a set of QueryMetrics impls to the core like "FullTopNQueryMetrics", "FullGroupByTopNQueryMetrics", etc., opposed to DefaultXxxQueryMetrics they emit everything for what method exist. Not that somebody want to use them as is, but they may serve as foundation for your private impls, which you may even derive from FullXxxQueryMetrics and just override a few methods with empty bodies.

[leventov]

The problem with adding dimensions/metrics emitted by default, is not that it harms e. g. us (we already use our custom QueryMetrics query impls, so it's indifferent), but that it's almost impossible to take them away in the future. The amount of metrics emitted by default is like entropy, never goes down. The existing set of dimensions emitted by default is already bad, e. g. intervals, segment, id almost doesn't have any practical sense.

Making own QueryMetrics has a non-trivial upfront cost (about a man-day) but Yahoo as a big user of Druid will have to do it anyway, sooner or later. But after the setup is done, the support is very easy.

[cheddar]

I'd be down with a "Full Metrics" type thing that we could turn on with a single config.

Funny thing about your examples of things that aren't useful, aside from interval, we use segment and id a lot in debugging our clusters...

[leventov]

Ok, I'll create a PR which adds "full metrics"

[akashdw]

Thanks @leventov, Should I include "full metrics" changes in this PR or I can do it in a separate PR? I also propose to include a blacklist metrics config, What do you think ?
I will create an issue to track and discuss "full metrics" + blacklist metrics config feature.

[leventov]

@akashdw I can post "full metrics" PR because essentially I already have it implemented internally (minus some specifics which I can remove). After that PR is merged, you could add blacklist configs to "FullMetrics" specifically, with defined semantics, not QueryMetrics "in general". E. g. it will look like

druid.query.topN.queryMetricsFactory=full
druid.query.topN.queryMetrics.full.excludeDimensions=[...]
druid.query.topN.queryMetrics.full.excludeMetrics=[...]

[akashdw]

Thanks @leventov, I will also update this PR to have an empty implementation of Granularity() method in the default implementation.

[akashdw]

@leventov added empty implementation for granularity() method.

[leventov]

Add comment "Don't emit by default". Same in other places

[leventov]

Please follow the 6-step procedure defined in the "Making subinterfaces of QueryMetrics for emitting custom dimensions and/or metrics for specific query types" section in QueryMetrics docs. Same for SelectQueryMetrics. Also, please update that section to use another query type in the example, because "Search" won't be a query time without specific QueryMetrics any more, as assumed in that doc.

[leventov]

@VisibleForTesting says almost the same, suggested to remove comment

[leventov]

Same

[leventov]

Also, consider extracting DataQueryMetrics (opposed to metadata queries) that could share some things, e. g. granularity.

[akashdw]

@leventov Addressed comments.

[leventov]

Please add Javadoc to this class like: This class is implemented with delegation to another QueryMetrics for compatibility, see "Making subinterfaces of QueryMetrics for emitting custom dimensions and/or metrics for specific query types" section in {@link QueryMetrics} javadoc

[leventov]

Consider naming "delegateQueryMetrics" or simply "delegate"

[leventov]

I think the body of this query() method should be empty, because query() should be already called on the provided delegate QueryMetrics, according to the GenericQueryMetricsFactory documentation. Please reflect this in a doc comment to the DefaultSearchQueryMetrics() constructor, like queryMetrics.query(query) must already be called on the provided queryMetrics

[leventov]

This also means that bodies of other methods, that are called from query(), should be empty or even throw IllegalStateException, to ensure there is no mistake

[leventov]

To be precise, I think query() should have empty body (with a comment), and the methods which used to be called from query() should throw ISE.

[leventov]

Please place this method along with other methods of it's group, i. e. after queryId().

[leventov]

Double search.search package

[akashdw]

search pkg contains another search pkg, https://github.com/druid-io/druid/tree/master/processing/src/main/java/io/druid/query/search/search.

[leventov]

Indeed, but seems like a error? I see no reason for that

[akashdw]

Yes, seems like an error. I will create a pr either to move all the classes from search.search package to search package or rename search.search pkg to something else ?

[leventov]

IMO just move from search.search to search

[akashdw]

👍

[leventov]

Same comments as for Search

[leventov]

This javadoc has different formatting that all other similar Javadocs, including SelectQueryMetricsFactory added in the same PR

[leventov]

Instead of referencing TopN, GroupBy and Timeseries as something that should not be taken as examples of following this procedure, in the end of the description of this procedure, please reference Search and Select as examples of this procedure implemented. Also please add notes about empty or exception throwing of query() and other "pre-query-execution-time" methods to this procedure.

[leventov]

@akashdw please don't rebase commits after making a PR, it breaks the thread. See https://github.com/druid-io/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master

[akashdw]

Thanks @leventov, sure won't rebase the commits from next time onwards.
Addressed comments, Please review.

[leventov]

@akashdw didn't you plan to make a PR that fixes search.search before continuing with this PR?

[akashdw]

@leventov created pr that fixes search.search #4785.

[akashdw]

@leventov Addressed comments and search package name correction pr got merged. Can you please review this PR.

[cheddar]

👍

[leventov]

@akashdw this PR broke compilation of the project. Please fix

[akashdw]

@leventov Having a look.

[akashdw]

@leventov QueryMetrics now has a new method, void identity(String identity); not implemented in DefaultSelectQueryMetrics and DefaultSearchQueryMetrics. Creating a pr for the fix.

[akashdw]

@leventov Created a pr for the fix #4906, please review.