Skip to content

Use customLabel if it exists#6511

Merged
spalger merged 6 commits into
elastic:masterfrom
ycombinator:gh-6407
Apr 5, 2016
Merged

Use customLabel if it exists#6511
spalger merged 6 commits into
elastic:masterfrom
ycombinator:gh-6407

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

Fixes #6407

},
avg: {
valProp: 'avg',
title: 'Average'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we want + label here too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps. I think it depends on what we expect users to put in for label. For instance, in the issue for this PR — #6407@tbragin used "SD" as the custom label. Assuming that's short for "standard deviation", it wouldn't make sense to include in the avg title, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, but you'd usually use the custom label to describe what sort of data the line represents. The design of the customLabel property really isn't ideal for multi value metrics unfortunately :-(

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 11, 2016

So i didn't even realize that adding "Standard Deviation" as a metric adds its own "Average" column. I'm not sure why... we have an explicit option for adding "Average" which has its own custom label (see screenshot).

Just brainstorming, here are some possible options I see:

(1) Apply the label entered for standard deviation custom label box in the "Lower Standard Deviation or [metric]" and "Upper Standard Deviation of [metric]" columns only by replacing only "Standard Deviation of [metric]"
(2) Alternatively, consider separate labels for each column if suggestion above doesn't work.
(3) Orthogonally, consider no longer add the "Average" column when user adds "Standard Deviation" relying on explicit configuration there. We can make this change in a major version as long as we release note it, I assume.

My preference would be doing (1) and optionally (3), but would love others to weigh in.

screen shot 2016-03-11 at 11 38 18 am

@rashidkpc
Copy link
Copy Markdown
Contributor

I believe the reason std deviation currently add its own average is because early versions of kibana didn't support more than one metric agg per chart, but now of course they do.

Agree with @tbragin, let's do 1, but also do 3.

@rashidkpc
Copy link
Copy Markdown
Contributor

Also, do 3 in a different pull

@ycombinator ycombinator self-assigned this Mar 11, 2016
@ycombinator
Copy link
Copy Markdown
Contributor Author

Thanks @rashidkpc and @tbragin. This PR implements (1) but I want to add a unit test for it. So removing review label for now.

@ycombinator
Copy link
Copy Markdown
Contributor Author

Unit test added (with much help from @Bargs — thank you!)

@ycombinator
Copy link
Copy Markdown
Contributor Author

jenkins, test it

@ycombinator ycombinator assigned spalger and unassigned ycombinator Apr 5, 2016
@spalger
Copy link
Copy Markdown
Contributor

spalger commented Apr 5, 2016

LGTM

@spalger spalger added the v5.0.0 label Apr 5, 2016
@spalger spalger merged commit 84a6bf7 into elastic:master Apr 5, 2016
@ycombinator ycombinator deleted the gh-6407 branch April 6, 2016 00:04
ycombinator added a commit that referenced this pull request Apr 25, 2016
Fix small mistake made in PR 6511
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants