Skip to content

Fix label on scripted field date histograms #8638

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.

Sign up for GitHub

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

Merged
Bargs merged 1 commit into from
Oct 12, 2016
Merged

Fix label on scripted field date histograms #8638

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ui/public/agg_types/buckets/date_histogram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 || '';
Copy link
Contributor

@thomasneirynck thomasneirynck Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweeto!

image

Copy link
Contributor

@thomasneirynck thomasneirynck Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Bargs Bargs Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd generally agree, but I'm playing it safe since we're so close to release with limited time to let this bake.

Copy link
Contributor

@epixa epixa Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Bargs Bargs Oct 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@epixa epixa Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@Bargs Bargs Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@epixa epixa Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this an issue in 4.x, or is this is a regression?

Copy link
Contributor Author

@Bargs Bargs Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's new and only affects painless/groovy/etc - the issue only affects the date histogram and lucene expressions can't produce date fields.

Copy link
Contributor

@epixa epixa Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it

return field + ' per ' + (output.metricScaleText || output.bucketInterval.description);
},
createFilter: createFilter,
decorateAggConfig: function () {
Expand Down