Backpressure metric should not be emitted by default

[leventov]

It undermines the whole idea behind the QueryMetrics system. See #6335 (comment)

[gianm]

I don't agree.

It is not fair to ask Druid users (who are often not developers in their own right) to write their own custom extension just to get basic metrics to be emitted. I think it's fair for "advanced" or "developer-oriented" metrics to require a QueryMetrics extension, but it's not fair for "user-oriented" metrics to require one. Furthermore, the metric, as designed in #6335, is not emitted unless you've actually enabled backpressure.

If we want to limit the number of metrics that are emitted by default, even when some of them would be useful to a user, it should be done via configuration, not by asking end users to write a custom extension.

[gianm]

I guess I don't understand the intended purpose of the QueryMetrics system. I thought it was to give 'advanced' cluster operations control over the nitty gritty of how metrics are emitted. I didn't think the intent was to require all end users to use it in order to configure which metrics get emitted and which don't.

[leventov]

The requirement of writing a custom extension ensures one important thing (this is reflected in the QueryMetrics javadoc): it allows "safe" backwards-incompatible changes, by means of breaking compilation of the custom QueryMetrics implementation via method addition, removal or rename. Currently, if some metric is added and emitted by default, it's existence, it's name and the output unit (time unit, for example) becomes irreversible liability forever.

Actually, this is the second "problem" noted here: #3954 (comment), see also the section "evolution of metrics" below.

Configuration path is suggested here: #4570 (comment) (except I revoke the promise of doing that myself). That type of configuration even could become the default, if new metrics are still not emitted by default, and in order of emitting them, the user must specify the name and the unit for them explicitly, a-la:

druid.query.queryMetrics.default.metrics.nodeBackPressureTime.name=query/node/backpressure
druid.query.queryMetrics.default.metrics.nodeBackPressureTime.unit=nanoseconds

During initialization of configuration, we could check that the metric "nodeBackPressureTime" still exists, and fail the app if it doesn't. It would also allow to evolve the metrics over time, if needed.

[jon-wei]

Some thoughts:

• Setting aside the issue of how to control whether a metric is emitted: for whether a new metric should be emitted by default or not, we should evaluate the new metric case-by-case, I don't think a blanket policy of "no new default metrics" is appropriate. This backpressure metric seems reasonable to emit by default to me.
• I agree with @gianm, I think it's excessive to require cluster operators to write an extension to control what metrics are emitted, this should be handled with configuration. A cluster operator is not necessarily a developer, and may not even have access to someone who could write such an extension. Control over what metrics are emitted seems like a fairly "shallow" interaction with the system that shouldn't require custom code.

[leventov]

@jon-wei

1. I strongly feel that no, no new per-query metric should be emitted by default whatsoever, and if we didn't have backwards compatibility constraints already, they should all have been opt-in. Explained why in QueryMetrics: abstraction layer of query metrics emitting (part of #3798) #3954 and granularity method in QueryMetrics. #4570. BTW interesting that right now, if somebody doesn't want the new metric, eg because their metric accepting system doesn't have extra capacity, there is no path to disable it other than writing a private extension.
2. Above I suggested the schema where no private extension needed, only 2 extra config lines.

[dclim]

Hi,

I also feel that requiring end-users to write a custom extension to be able to receive helpful metrics is too high of a barrier for usage and I would favor a configuration-based approach. I'm okay with having the custom extension mechanism be available for advanced users to fine-tune what is emitted, but expect that most Druid users would be happier with and would have an easier time understanding a configuration-based system.

I also don't feel that this issue should be a blocker to 0.13.0, since this additional metric is only emitted if druid.broker.http.maxQueuedBytes is set to a non-default value. Likely if a user is having an issue with backpressure, they would also welcome visibility into how the backpressure system is operating (via this metric). However I agree that it would be good to have the ability to enable/disable individual metrics by configuration.

[leventov]

@dclim your message has the same arguments as @gianm's and @jon-wei's, and still don't answer to my points:

• I DON'T suggest users to write custom extensions to enable the metric
• there are multiple issues with emitting query metrics by default, and the fact that it won't be emitted unless some other non-default configuration is set doesn't really void any of them.

[jon-wei]

My view is that just by defining a metric as something that can be emitted under certain conditions, with a value representing some concept, we are left with backwards compatibility concerns. Even if a new metric is not emitted by default, some fraction of the userbase will enable and use it, and will be affected by compatibility breaking changes.

So my main concern with adding a new a metric as a default would be cost of emission/storage vs. the usefulness of the metric. In this case, the backpressure metric is extremely useful if you have the feature enabled, so I don't see an issue with enabling it by default. There is some context dependence and subjectivity to this tradeoff, so I do think we need an easy way to control what metrics are emitted.

BTW interesting that right now, if somebody doesn't want the new metric, eg because their metric accepting system doesn't have extra capacity, there is no path to disable it other than writing a private extension.

An easy means for controlling what metrics can be emitted through configuration sounds great to me, but I'm also not convinced that we should block the 0.13.0 release on adding a configuration property to control this specific metric.

I think that should be better done as part of a general patch that covers all metrics in a later release. Adding a property now that may or may not fit in with the design of a larger system also imposes backwards compatibility concerns. It's also a bit strange from a user experience perspective to have a single config like this that doesn't have any counterpart for other metrics.

The new metric is only emitted when the user upgrades and explicitly enables a new feature, so I'm not too concerned that people's metrics collection will suddenly be overwhelmed by using 0.13.0, if we make it clear in the release notes that this new per-query metric would be emitted. Depending on collector as well, there may be the option of filtering out specific metrics if you don't want them (though there will still be the network load). I would also expect "I want backpressure, but I don't want the backpressure metric" to be a less common case.

[gianm]

For 0.13.0's default config I am fine keeping the new metric, or removing it (either way I'm going to enable it at my site). But after 0.13.0 I think we need a better way of handling this. I don't think it's user-friendly to ask users, especially new users, to write private extensions or even long configuration files in order to get metrics emission to work. There are two questions packed in here.

First, what metrics do we emit by default? It could be some "reasonable" level, it could be everything, or it could be nothing. @jon-wei and I have advocated for "reasonable". @leventov has said he would prefer "nothing", but in a concession to backwards compatibility, is ok with "the level that was in place when the QueryMetrics system was set up".

Second, how do cluster operators customize metrics emitting? Today, they have to write a private extension. It sounds like everyone things configuration would be a good option, but it's not implemented yet.

I would like to propose the following.

• Adopt the "reasonable" standard for new metrics (if a new user comes in, with zero configuration, they get this "reasonable" set). The names of these metrics and metric-dimensions are part of Druid's public API. Other metrics and metric-dimensions, that are not in the default set, are not in Druid's public API.
• Continue to support QueryMetrics extensions as a way for advanced operators to have full control over how metrics emission works. It will work the same way as it works now. If a new kind of metric or metric-dimension is added, then QueryMetrics extension authors will still be 'notified' and have the opportunity to decide what to do about it.
• Consider building a configuration-based approach that lets intermediate users do some customizations through config only.

I feel that this approach will provide stability for a core set of most-useful metrics, while still allowing customizability, and allowing some mechanism to add "advanced" metrics that aren't part of the public API, if we want to do that for some reason.

Btw, with regard to the suggestion below for how configuration works, I don't see how it adds value beyond making the name "query/node/backpressure" part of the public API of Druid. It eliminates that, but introduces a new thing "nodeBackPressureTime" that is part of the public API (we can't change it without breaking old configs).

druid.query.queryMetrics.default.metrics.nodeBackPressureTime.name=query/node/backpressure
druid.query.queryMetrics.default.metrics.nodeBackPressureTime.unit=nanoseconds

[leventov]

@gianm the advantage of druid.query.queryMetrics.default.metrics.nodeBackPressureTime.name style configs is that a user must specify them explicitly in order to get this metric. Then, if one day we migrate to a different network comm mechanism and nodeBackPressureTime becomes irrelevant, we could remove this thing and add another metric druid.query.queryMetrics.default.metrics.somethingCompletelyDifferent for network comm health telemetry. Then user's configs become invalid, and Druid will fail to start with an exception with old config. It will force users to recognize that the metric has been removed.

What is possible with "reasonable" metrics emitted by default being public API:

1. Users depend on those metrics and set up alerts for them exceeding some thresholds.
2. At some point we remove those metrics, or change their units, or resolution, or due to some architectural changes in Druid "normal" and "ill" values for the metric become completely different.
3. Users don't notice that, their alerting dashboards keep silence because they are happy with the metrics being apparently zero or very low.

Also, again, the reasonable per-query metric could significantly increase the total metric volume, so that the metric collecting system becomes overloaded, or if this is a commercial SaaS service, the cost is suddenly up.

So ultimately I don't believe this is a safe default path. We should assume that users don't read the documentation at all, or skip some parts of it arbitrarily.

I agree there could be a non-default metrics option "reasonable", the doc of which (assuming that if users set some option in their config, they read at least the doc of that particular option) says that:

• The emitted metrics are not public Druid API, they could be renamed, revoked, unit changed, etc. at any release;
• Setting up alerts based on those metrics, be prepared to re-check each metric on each Druid update;
• The amount of emitted "reasonable" metrics could significantly increase on a Druid update; the metric collecting system could get choked or become more expensive.

If a user reads this and agrees with this, then fine.

Consider building a configuration-based approach that lets intermediate users do some customizations through config only.

I think it's not something just to consider, it should be in place and should be the default.

[gianm]

@gianm the advantage of druid.query.queryMetrics.default.metrics.nodeBackPressureTime.name style configs is that a user must specify them explicitly in order to get this metric. Then, if one day we migrate to a different network comm mechanism and nodeBackPressureTime becomes irrelevant, we could remove this thing and add another metric druid.query.queryMetrics.default.metrics.somethingCompletelyDifferent for network comm health telemetry. Then user's configs become invalid, and Druid will fail to start with an exception with old config. It will force users to recognize that the metric has been removed.
…
I agree there could be a non-default metrics option "reasonable", the doc of which (assuming that if users set some option in their config, they read at least the doc of that particular option) says that:

• The emitted metrics are not public Druid API, they could be renamed, revoked, unit changed, etc. at any release;
• Setting up alerts based on those metrics, be prepared to re-check each metric on each Druid update;
• The amount of emitted "reasonable" metrics could significantly increase on a Druid update; the metric collecting system could get choked or become more expensive.

I just don't think this method is a user-friendly way of handling metrics. It means that users won't get metrics unless they manually specify a line of configuration for each one. New users don't generally know what metrics are useful until they run into some problem that metrics help debug. At that point, it would be best if the metric were already available in their metrics collection system, rather than requiring the user to go back and turn it on with a cluster redeploy. Requiring users to think about each metric ahead of time is a setup burden.

I also think it would be better to abandon the desire to be able to change any query metrics on any release. We should think carefully about new metrics we add to the "public API" set, and then once we add them, don't change their names or units. We should feel free retire them if they become obsolete (like in your example of using a totally different network communication mechanism), but we should avoid changing their names or meanings. I don't think a mechanism to automatically notify users of such changes and force them to deal with them is the right solution, since that still makes upgrading a tougher process, which we should be trying to avoid.

I think it's also best if the "public API" set of metrics is relatively large, as a percentage of overall metrics. It makes the system even more user friendly and easier to upgrade.

Also, again, the reasonable per-query metric could significantly increase the total metric volume, so that the metric collecting system becomes overloaded, or if this is a commercial SaaS service, the cost is suddenly up.

I definitely have a lot more sympathy for this concern than for the concern about wanting to make backwards-incompatible changes. I think that we can address this metrics size concern by having it be part of the thought process for adding new metrics. I don't think the thought process needs to be as severe as "no new metric-dimensions by default, ever" but I can see it making sense to avoid adding any new metrics to the default set. Since new metrics mean big increases in volumes, but new metric-dimensions are relatively small increases, and can be worth it.

My experience has been that the per-segment metrics are the vast majority of bulk of query metrics, by volume. I am down for a policy that we shouldn't add more of those by default. I would also be fine with removing them from the default set, if we had an easy configuration-based way to turn them back on. I have definitely seen those per-segment metrics overwhelm peoples' metric collection systems, so yeah, agreed that is a real issue.

Concretely, I could see it making sense to do the following.

• Not include the backpressure metric in the default set.
• Maybe even remove per-segment metrics from the default set?
• Create a configuration based approach that lets people set their query metrics to "none" or "default" or "all". Maybe even more customization than that, I dunno.
• Allow the "default" set to evolve over time to add new dimensions if they are broadly useful, but avoid adding new metrics to the default set. The goal is to avoid increasing the size of metrics by too much. I think "identity" is a good example of a metric that makes sense to add to the default set. It is broadly useful to anyone that enabled authentication, and to people that didn't, it always has the same value so will compress well.
• Consider metric names and units part of Druid's public API and avoid changing them (but feel free to remove obsolete ones).
• Keep doing the QueryMetrics thing to allow full customization and force-notification for people that want that.

[leventov]

Sounds like a good plan to me.