-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Limit visualization editor based on rollup capabilities #23637
Limit visualization editor based on rollup capabilities #23637
Conversation
…aw rollup docs can be viewed
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this weird error when creating a visualization:
The request does include max and min aggs. We ran into this problem before didn't we?
[
{
"index":"test_rollup_index",
"query":{
"size":0,
"aggregations":{
"maxAgg":{
"max":{
"field":"memory"
}
},
"minAgg":{
"min":{
"field":"memory"
}
}
},
"query":{
"bool":{
"must":[
{
"match_all":{}
},
{
"range":{
"utc_time":{
"gte":1538418388395,
"lte":1538432788395,
"format":"epoch_millis"
}
}
}
],
"filter":[],
"should":[],
"must_not":[]
}
}
}
}
]
Here's the job config:
{
"config": {
"id": "test rollup job",
"index_pattern": "log*",
"rollup_index": "test_rollup_index",
"cron": "0 * * * * ?",
"groups": {
"date_histogram": {
"interval": "1m",
"field": "utc_time",
"time_zone": "UTC"
},
"histogram": {
"interval": 100,
"fields": [
"machine.ram",
"meta.user.lastname",
"id",
"bytes",
"phpmemory",
"memory"
]
},
"terms": {
"fields": [
"geo.srcdest",
"id",
"longValuesWithSpaces.keyword"
]
}
},
"metrics": [
{
"field": "id",
"metrics": [
"min"
]
},
{
"field": "meta.user.lastname",
"metrics": [
"min"
]
},
{
"field": "memory",
"metrics": [
"min"
]
},
{
"field": "phpmemory",
"metrics": [
"min"
]
},
{
"field": "machine.ram",
"metrics": [
"min"
]
}
],
"timeout": "20s",
"page_size": 100
},
"status": {
"job_state": "started",
"current_position": {
"bytes.histogram": 700,
"geo.srcdest.terms": "BF:PT",
"id.histogram": null,
"id.terms": null,
"longValuesWithSpaces.keyword.terms": null,
"machine.ram.histogram": 11811160000,
"memory.histogram": null,
"meta.user.lastname.histogram": null,
"phpmemory.histogram": null,
"utc_time.date_histogram": 1538433000000
},
"upgraded_doc_id": true
},
"stats": {
"pages_processed": 142,
"documents_processed": 8600,
"rollups_indexed": 8600,
"trigger_count": 32
}
}
) : null; | ||
} | ||
|
||
renderNoDefaultMessage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do I need to do to get this to display? I tried deleting the default index pattern but then it just reassigned another one as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have no index patterns defined
|
||
import { Header } from './components/header'; | ||
import { List } from './components/list'; | ||
|
||
export class IndexPatternList extends Component { | ||
static propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to keep the propTypes around?
IndexPatternList.propTypes = {
indexPatternCreationOptions: PropTypes.array.isRequired,
defaultIndex: PropTypes.string,
indexPatterns: PropTypes.array.isRequired,
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could, but the propTypes are enforced by components that this component renders so I don't think it's necessary
}, | ||
interval: { | ||
base: interval, | ||
help: `Must be a multiple of rollup configuration interval: ${interval}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1m is actually a calendar interval, whereas 10m is fixed 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, of course! I think we'll need some improved error reporting here in the next release.
@@ -17,6 +17,6 @@ aggTypeFilters.addFilter( | |||
} | |||
const aggName = aggType.name; | |||
const aggs = indexPattern.typeMeta.aggs; | |||
return Object.keys(aggs).includes(aggName); | |||
return aggName === 'count' || Object.keys(aggs).includes(aggName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this merit a comment since it looks like a special case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, will add one
@cjcenizal I was able to replicate your error. In this case, it's the result of the queried histogram field ( |
@cjcenizal I added a catch for the min/max aggs: https://github.com/elastic/kibana/pull/23637/files#diff-1be9dd2b7946d5364296485200c35927R100 This will allow the visualization to render without having the min/max information. I think a more complete solution would be when the user selects a histogram field in the job wizard, we automatically select the same field in metrics as well with min/max aggs preselected, maybe with a flag saying that this will allow for a better visualization experience. This could get a little tricky if the user deletes the histogram field (do we keep the metrics one around?) and other back and forth cases. I think it would be a good enhancement for phase 2. |
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the new catch
and it works great! Thanks for digging into that. Code LGTM!
.catch(() => { | ||
toastNotifications.addWarning(` | ||
Unable to retrieve max and min values to auto-scale histogram buckets. | ||
This may lead to poor visualization performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a performance issue? Unless I'm misunderstanding, it seems like it's more of a problem with the veracity of the visualization's results. Would "This may lead to incorrect visualization results" be more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually to increase the size of the buckets in case the given minimum interval causes too many buckets, see this comment. when I tested using a rollup field that doesn't have min/max after adding the catch
, I get a visualization that looks like this:
if it had min/max, the interval would have been increased to decrease the number of bars. this example loaded fine for me, but I imagine a greater number of bars could be problematic on certain machines. hence performance issue 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks!
* Limit vis editor based on rollup capabilities * Migrate new `type` and `typeMeta` field mappings for old indices * Disable timefield for rollup index patterns in discover so that the raw rollup docs can be viewed * Restructure index pattern list registry * Disable scripted fields tab for rollup index patterns * Allow count in list of available vis aggs * Add error when min/max aggs are not available for histograms
* Limit vis editor based on rollup capabilities * Migrate new `type` and `typeMeta` field mappings for old indices * Disable timefield for rollup index patterns in discover so that the raw rollup docs can be viewed * Restructure index pattern list registry * Disable scripted fields tab for rollup index patterns * Allow count in list of available vis aggs * Add error when min/max aggs are not available for histograms
* Limit vis editor based on rollup capabilities * Migrate new `type` and `typeMeta` field mappings for old indices * Disable timefield for rollup index patterns in discover so that the raw rollup docs can be viewed * Restructure index pattern list registry * Disable scripted fields tab for rollup index patterns * Allow count in list of available vis aggs * Add error when min/max aggs are not available for histograms
PR also:
type
andtypeMeta
field mappings for old indices (using new Index Migration feature)count
in visualization editorRollup documents in Discover