Skip to content
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

Removes the "Index contains time-based events" checkbox #11409

Merged
merged 13 commits into from
Apr 26, 2017
Merged

Removes the "Index contains time-based events" checkbox #11409

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
203c89c
Removing "Index contains time-based events" checkbox
ycombinator Apr 25, 2017
997adf7
Rename label
ycombinator Apr 25, 2017
4064c99
Adding "No Time Filter field" to time field select list
ycombinator Apr 25, 2017
df0e18b
Using translation for i18n and consistency with label on create form
ycombinator Apr 25, 2017
dba8583
More useful verbiage
ycombinator Apr 25, 2017
442467a
Updating copy
ycombinator Apr 25, 2017
5348a55
Adding help block below select for added clarity
ycombinator Apr 25, 2017
d60eda0
Removing functional tests that used time-based events checkbox
ycombinator Apr 25, 2017
244cb72
s/INDEX/INDICES/ to be reflect reality
ycombinator Apr 25, 2017
c3270a6
Fixing logic error
ycombinator Apr 25, 2017
3688f12
Updating functional test to match new behavior
ycombinator Apr 25, 2017
a1c86d7
Use controller property instead of inline expression
ycombinator Apr 26, 2017
f701b58
More readable variable name
ycombinator Apr 26, 2017
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
22 changes: 6 additions & 16 deletions .../kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<div class="form-group">
<label translate="KIBANA-INDEX_NAME_OR_PATTERN"></label>
<p class="help-block" ng-if="!controller.newIndexPattern.nameIsPattern" translate="KIBANA-WILDCARD_DYNAMIC_INDEX_PATTERNS"></p>
<p class="help-block" ng-if="controller.newIndexPattern.isTimeBased && controller.newIndexPattern.nameIsPattern"><span translate="KIBANA-STATIC_TEXT_IN_DYNAMIC_INDEX_PATTERNS"></span> &mdash;
<p class="help-block" ng-if="controller.newIndexPattern.nameIsPattern"><span translate="KIBANA-STATIC_TEXT_IN_DYNAMIC_INDEX_PATTERNS"></span> &mdash;
<small><a target="_blank" href="http://momentjs.com/docs/#/displaying/format/" translate="KIBANA-DATE_FORMAT_DOCS"></a></small></p>
<input
data-test-subj="createIndexPatternNameInput"
Expand All @@ -41,19 +41,9 @@
</small>
</div>

<div class="form-group time-and-pattern">
<label>
<input
data-test-subj="createIndexPatternIsTimeBasedCheckBox"
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to update the functional tests to remove references to this element.

ng-model="controller.newIndexPattern.isTimeBased"
type="checkbox">
<span translate="KIBANA-CONTAINS_TIME_BASED_EVENTS"></span>
</label>
</div>

<div class="form-group" ng-if="controller.newIndexPattern.isTimeBased">
<div class="form-group">
<label>
<span translate="KIBANA-TIME_FIELD_NAME"></span>
<span translate="KIBANA-TIME_FILTER_FIELD_NAME"></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: what does time filter mean in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using that per this suggestion: #10444 (comment).

I think "time picker" might be more appropriate but I'm not sure if users know what that is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I guess "time filter" works. I like "time picker" because I know what that is, but I'm afraid that might couple these field too closely to a particular UI component.

&nbsp;
<kbn-info info="{{ 'KIBANA-FIELD_FILTER_EVENTS_GLOBAL_TIME' | translate }}"></kbn-info>
&nbsp;
Expand All @@ -63,7 +53,7 @@
</label>
<select
data-test-subj="createIndexPatternTimeFieldSelect"
ng-disabled="controller.fetchFieldsError"
ng-disabled="controller.fetchFieldsError || (controller.dateFields.length === 1)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be clearer to use !controller.doesIndexHaveDateFields here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that was introduced later and should definitely be used here. Thanks!

ng-required="!controller.fetchFieldsError"
ng-options="field.name for field in controller.dateFields"
ng-model="controller.newIndexPattern.timeField"
Expand Down Expand Up @@ -92,7 +82,7 @@
</div>

<div class="form-group time-and-pattern">
<label ng-if="controller.newIndexPattern.isTimeBased">
<label>
<input
data-test-subj="createIndexPatternNameIsPatternCheckBox"
ng-model="controller.newIndexPattern.nameIsPattern"
Expand All @@ -103,7 +93,7 @@
</label>
</div>

<div class="form-group" ng-if="controller.newIndexPattern.isTimeBased && controller.newIndexPattern.nameIsPattern">
<div class="form-group" ng-if="controller.newIndexPattern.nameIsPattern">
<div class="alert alert-warning">
</p>
<h4 translate="KIBANA-ALERT_INDEX_PATTERN_DEPRECATED"></h4>
Expand Down
49 changes: 31 additions & 18 deletions ...ns/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ uiModules.get('apps/management')
// Configure the new index pattern we're going to create.
this.newIndexPattern = {
name: config.get('indexPattern:placeholder'),
isTimeBased: true,
nameIsPattern: false,
expandable: false,
nameInterval: _.find(intervals, { name: 'daily' }),
Expand All @@ -40,9 +39,18 @@ uiModules.get('apps/management')
this.patternErrors = [];
this.fetchFieldsError = $translate.instant('KIBANA-LOADING');

const TIME_FILTER_FIELD_OPTIONS = {
NO_DATE_FIELD_SELECTED: {
name: $translate.instant('KIBANA-NO_DATE_FIELD_SELECTED')
},
NO_DATE_FIELDS_IN_INDEX: {
name: $translate.instant('KIBANA-NO_DATE_FIELDS_IN_INDEX')
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor suggestion: technically, we're referring to "indices", and not a single index, right? It all depends on how many indices match the index pattern. Maybe it's just me, but when I first started using Kibana, it took awhile for me to understand the difference between index pattern and index. I think renaming these to be plural will help reinforce the relationship, e.g. NO_DATE_FIELDS_IN_INDICES and KIBANA-NO_DATE_FIELDS_IN_INDICES.

}
};

const fetchFieldList = () => {
this.dateFields = this.newIndexPattern.timeField = null;
const useIndexList = this.newIndexPattern.isTimeBased && this.newIndexPattern.nameIsPattern;
const useIndexList = this.newIndexPattern.nameIsPattern;
let fetchFieldsError;
let dateFields;

Expand Down Expand Up @@ -89,7 +97,17 @@ uiModules.get('apps/management')

const updateFieldList = results => {
this.fetchFieldsError = results.fetchFieldsError;
this.dateFields = results.dateFields;
if (this.fetchFieldsError) {
return;
}

this.dateFields = results.dateFields || [];
if (this.dateFields.length === 0) {
this.dateFields.unshift(TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX);
} else {
this.dateFields.unshift(TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED);
}
this.newIndexPattern.timeField = this.dateFields[0];
};

const updateFieldListAndSetTimeField = (results, timeFieldName) => {
Expand All @@ -100,12 +118,13 @@ uiModules.get('apps/management')
}

const matchingTimeField = results.dateFields.find(field => field.name === timeFieldName);
const defaultTimeField = results.dateFields[0];

//assign the field from the results-list
//angular recreates a new timefield instance, each time the list is refreshed.
//This ensures the selected field matches one of the instances in the list.
this.newIndexPattern.timeField = matchingTimeField ? matchingTimeField : defaultTimeField;
if (matchingTimeField) {
this.newIndexPattern.timeField = matchingTimeField;
}
};

const resetIndex = () => {
Expand Down Expand Up @@ -168,7 +187,7 @@ uiModules.get('apps/management')
this.canExpandIndices = () => {
// to maximize performance in the digest cycle, move from the least
// expensive operation to most
return this.newIndexPattern.isTimeBased && !this.newIndexPattern.nameIsPattern && _.includes(this.newIndexPattern.name, '*');
return !this.newIndexPattern.nameIsPattern && _.includes(this.newIndexPattern.name, '*');
};

this.refreshFieldList = () => {
Expand All @@ -184,14 +203,15 @@ uiModules.get('apps/management')

this.createIndexPattern = () => {
const id = this.newIndexPattern.name;
const timeFieldName =
this.newIndexPattern.isTimeBased
? this.newIndexPattern.timeField.name
: undefined;
let timeFieldName;
if ((this.newIndexPattern.timeField !== TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED)
&& (this.newIndexPattern.timeField !== TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be reduced to:

![
  TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED,
  TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX
].includes(this.newIndexPattern.timeField)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I considered something like that but I find it less readable. In particular, I found the .includes relationship to be inverted (in terms of readability).

I'd be fine with shortening this if we could express this in reverse, so something like (pseudocode):

!this.newIndexPattern.timeField.isOneOf([ ... ])

But I don't think there's any way to do that, either in JS itself or using a helper library.

So I'd prefer to leave it the way it-is for added readability.

If you strongly disagree, we can get a third person to break the tie.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's all good, I don't feel very strongly about this.

timeFieldName = this.newIndexPattern.timeField.name;
}

// Only event-time-based index patterns set an intervalName.
const intervalName =
this.newIndexPattern.isTimeBased && this.newIndexPattern.nameIsPattern
this.newIndexPattern.nameIsPattern
? this.newIndexPattern.nameInterval.name
: undefined;

Expand Down Expand Up @@ -228,11 +248,9 @@ uiModules.get('apps/management')
};

$scope.$watchMulti([
'controller.newIndexPattern.isTimeBased',
'controller.newIndexPattern.nameIsPattern',
'controller.newIndexPattern.nameInterval.name'
], (newVal, oldVal) => {
const isTimeBased = newVal[0];
const nameIsPattern = newVal[1];
const newDefault = getDefaultPatternForInterval(newVal[2]);
const oldDefault = getDefaultPatternForInterval(oldVal[2]);
Expand All @@ -241,10 +259,6 @@ uiModules.get('apps/management')
this.newIndexPattern.name = newDefault;
}

if (!isTimeBased) {
this.newIndexPattern.nameIsPattern = false;
}

if (!nameIsPattern) {
delete this.newIndexPattern.nameInterval;
delete this.newIndexPattern.timeField;
Expand Down Expand Up @@ -301,7 +315,6 @@ uiModules.get('apps/management')
});

$scope.$watchMulti([
'controller.newIndexPattern.isTimeBased',
'controller.sampleCount'
], () => {
this.refreshFieldList();
Expand Down
2 changes: 1 addition & 1 deletion ...gins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

<p ng-if="indexPattern.timeFieldName" class="kuiText kuiVerticalRhythm">
<span class="label label-success">
<i class="fa fa-clock-o"></i> Configured time field: {{indexPattern.timeFieldName}}
<i class="fa fa-clock-o"></i><span translate="KIBANA-TIME_FILTER_FIELD_NAME"></span>: {{indexPattern.timeFieldName}}
</span>
</p>

Expand Down
5 changes: 3 additions & 2 deletions src/core_plugins/kibana/translations/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"UI-WELCOME_ERROR": "Kibana did not load properly. Check the server output for more information.",
"KIBANA-CONFIGURE_INDEX_PATTERN": "Configure an index pattern",
"KIBANA-MUST_CONFIGURE_INDEX_PATTERN": "In order to use Kibana you must configure at least one index pattern. Index patterns are used to identify the Elasticsearch index to run search and analytics against. They are also used to configure fields.",
"KIBANA-CONTAINS_TIME_BASED_EVENTS": "Index contains time-based events",
"KIBANA-INDEX_NAME_CREATED_BY_EVENT_TIMES": "Use event times to create index names ",
"KIBANA-DEPRECATED": "[DEPRECATED]",
"KIBANA-ALERT_INDEX_PATTERN_DEPRECATED": "Time-interval based index patterns are deprecated!",
Expand All @@ -30,7 +29,9 @@
"KIBANA-EXISTING_MATCH_PERCENT": "Pattern matches {{indexExistingMatchPercent}} of existing indices and aliases",
"KIBANA-NON_MATCHING_INDICES_AND_ALIASES": "Indices and aliases that were found, but did not match the pattern:",
"KIBANA-MORE": "more",
"KIBANA-TIME_FIELD_NAME": "Time-field name",
"KIBANA-TIME_FILTER_FIELD_NAME": "Time Filter field name",
"KIBANA-NO_DATE_FIELD_SELECTED": "No date field selected",
"KIBANA-NO_DATE_FIELDS_IN_INDEX": "No date fields in index",
"KIBANA-REFRESH_FIELDS": "refresh fields",
"KIBANA-INVALID_INDEX_PATTERN": "Invalid index name pattern.",
"KIBANA-DATE_FORMAT_DOCS": "Date Format Documentation",
Expand Down