Fix label on scripted field date histograms #8638
Conversation
Now that we support Painless scripted fields users can create scripted date fields, and thus scripted date histograms. The label making method for the date histogram agg was getting the field name in a way that was incompatible with scripted fields, so I've added some fallback code for that scenario. I looked through the rest of the makeLabel methods on all the other aggs and they all correctly access the field displayName already so this should only need fixed for date histograms. Fixes elastic#8632
|
@stacey-gammon mind reviewing this one as well while you're working on scripted fields? I'd be happy to trade when you have a PR ready for the scripted field filter bug. |
| @@ -41,7 +41,8 @@ export default function DateHistogramAggType(timefilter, config, Private) { | |||
| makeLabel: function (agg) { | |||
| const output = this.params.write(agg); | |||
| const params = output.params; | |||
| return params.field + ' per ' + (output.metricScaleText || output.bucketInterval.description); | |||
| const field = params.field || agg.params.field.displayName || ''; | |||
| @@ -41,7 +41,8 @@ export default function DateHistogramAggType(timefilter, config, Private) { | |||
| makeLabel: function (agg) { | |||
| const output = this.params.write(agg); | |||
| const params = output.params; | |||
| return params.field + ' per ' + (output.metricScaleText || output.bucketInterval.description); | |||
| const field = params.field || agg.params.field.displayName || ''; | |||
There was a problem hiding this comment.
I couldn't trigger a single example where the agg.params.field.displayName was empty. Shouldn't we just use that instead of using the cascade?
There was a problem hiding this comment.
I'd generally agree, but I'm playing it safe since we're so close to release with limited time to let this bake.
There was a problem hiding this comment.
I worry about accessing the field display name directly like this because elsewhere in this file we access params information in a particularly defensive way: https://github.com/elastic/kibana/pull/8638/files#diff-2cd930ccd69e832107c9578ef648e5b7R23
That leads me to believe that there are circumstances where agg doesn't have the nested params data, which would cause this code to trigger an error.
There was a problem hiding this comment.
Most other aggs (example) access displayName directly, so it seemed safe to me. I suppose there might something special about the date histogram though. I can change it to use lodash's get if you'd like.
There was a problem hiding this comment.
@Bargs I'm comfortable with this approach if we want to roll it out for 5.x, since that'll be given a full blown QA run, but if we want to get this into 5.0, we should be more cautious around the access here.
There was a problem hiding this comment.
@epixa makes sense, it's a trivial change so the question is, do we want to put it in 5.0? If so I'm happy to make the change
There was a problem hiding this comment.
Was this an issue in 4.x, or is this is a regression?
There was a problem hiding this comment.
It's new and only affects painless/groovy/etc - the issue only affects the date histogram and lucene expressions can't produce date fields.
--------- **Commit 1:** Fix label on scripted field date histograms Now that we support Painless scripted fields users can create scripted date fields, and thus scripted date histograms. The label making method for the date histogram agg was getting the field name in a way that was incompatible with scripted fields, so I've added some fallback code for that scenario. I looked through the rest of the makeLabel methods on all the other aggs and they all correctly access the field displayName already so this should only need fixed for date histograms. Fixes #8632 * Original sha: 7359b45 * Authored by Matthew Bargar <[email protected]> on 2016-10-12T15:10:15Z
|
@epixa let me know if I should backport to 5.0 |
|
@Bargs I left a post-merge inline comment. Any thoughts you have on that would be appreciated. |
--------- **Commit 1:** Fix label on scripted field date histograms Now that we support Painless scripted fields users can create scripted date fields, and thus scripted date histograms. The label making method for the date histogram agg was getting the field name in a way that was incompatible with scripted fields, so I've added some fallback code for that scenario. I looked through the rest of the makeLabel methods on all the other aggs and they all correctly access the field displayName already so this should only need fixed for date histograms. Fixes elastic#8632 * Original sha: 9bed93b73c0f3cc90dc1fa2889aa6b18acab402b [formerly 7359b45] * Authored by Matthew Bargar <[email protected]> on 2016-10-12T15:10:15Z Former-commit-id: de68ed0
[backport] PR elastic#8638 to 5.x Former-commit-id: 4048df6
Now that we support Painless scripted fields users can create scripted
date fields, and thus scripted date histograms. The label making method
for the date histogram agg was getting the field name in a way that was
incompatible with scripted fields, so I've added some fallback code for
that scenario. I looked through the rest of the makeLabel methods on all
the other aggs and they all correctly access the field displayName
already so this should only need fixed for date histograms.
Fixes #8632