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

Don't save the current timezone in visualizations #34795

Merged
merged 7 commits into from
Apr 11, 2019
+164 −0
Merged

Don't save the current timezone in visualizations #34795

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
816d442
Don't save the current timezone in visualizations
timroes Apr 9, 2019
0ae6ed0
Add additional test
timroes Apr 9, 2019
8c91d8e
Add test and switch migration number
timroes Apr 9, 2019
217a9ad
Merge branch 'master' into fix-date-histogram-tz
timroes Apr 9, 2019
53cda84
Don't clone object according to review feedback
timroes Apr 10, 2019
91beb2f
Better documentation
timroes Apr 10, 2019
6a2dcd2
Update src/legacy/core_plugins/kibana/migrations.js
lukeelmers Apr 11, 2019
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
30 changes: 30 additions & 0 deletions src/legacy/core_plugins/kibana/migrations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,34 @@ function migrateIndexPattern(doc) {
doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource);
}

function removeDateHistogramTimeZones(doc) {
const newDoc = cloneDeep(doc);
Copy link
Member

Choose a reason for hiding this comment

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

nit: Not a huge thing, but you don't necessarily need to clone every doc that comes in since you are only going to be updating attributes.visState on date histograms. You could return early if the doc is not identified as needing to be changed, and only clone / reassign the visState on docs that you know you need to touch.

const visStateJSON = get(doc, 'attributes.visState');
if (visStateJSON) {
let visState;
try {
visState = JSON.parse(visStateJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
}
if (visState && visState.aggs) {
visState.aggs.forEach(agg => {
// We're checking always for the existance of agg.params here. This should always exist, but better
// be safe then sorry during migrations.
if (agg.type === 'date_histogram' && agg.params) {
delete agg.params.time_zone;
}

if (get(agg, 'params.customBucket.type', null) === 'date_histogram' && agg.params.customBucket.params) {
delete agg.params.customBucket.params.time_zone;
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: lodash omit could save you from some conditionals here, e.g.

if (agg.type === 'date_histogram' || get(agg, 'params.customBucket.type') === 'date_histogram') {
  agg.params = omit(agg.params, ['time_zone', 'customBucket.params.time_zone']);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer that? I find it actually harder to read, then the more explicit version :-)

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 either is fine. Go with what you think is most obvious.

Copy link
Member

Choose a reason for hiding this comment

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

I marked it as a nit for a reason 😉 Totally up to your preference here, I only suggested it because omit will automatically skip things that are undefined.

Copy link
Member

@lukeelmers lukeelmers Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

(And FWIW I had no issues groking the code as it is currently written)

});
newDoc.attributes.visState = JSON.stringify(visState);
}
}
return newDoc;
}

export const migrations = {
'index-pattern': {
'6.5.0': (doc) => {
Expand All @@ -66,6 +94,7 @@ export const migrations = {
}
},
visualization: {
'6.7.0': removeDateHistogramTimeZones,
'7.0.0': (doc) => {
// Set new "references" attribute
doc.references = doc.references || [];
Expand Down Expand Up @@ -146,6 +175,7 @@ export const migrations = {
throw new Error(`Failure attempting to migrate saved object '${doc.attributes.title}' - ${e}`);
}
},
'7.0.1': removeDateHistogramTimeZones,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylersmalley I already talked to Chris, but we wanted to clarify with you that our thoughts were correct:

In the backport to 6.7, I'll remove this 7.0.1 migration and just leave the 6.7 migration in. But since we already released 7.0, and potentially have now people on that version that should get the same fix, we need both 6.7.2 and 7.0.1 to do the migrations. Also just adding a 7.0.1 wouldn't be good, since if we backport this to 6.7 a user who runs the 7.0.1 migration on 6.7 would then, after upgrading to 7.0.1+, not get all the 7.0.0 migrations.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should only be one - one of these needs to be removed. It doesn't necessarily have to follow the versioning of Kibana. On startup, it will see if there are any newer versions for the visualization type, we will run those migrations to get up to that version.

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 there needs to be 2, because some clients will have run the 7.0.0 migrations already, but some will not have (those running 6.x). And we want to fix both people without forcing the 6.x people to upgrade to 7.x. Does that make sense, @tylersmalley

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked about that offline and it makes sense now to everyone. I updated the code to have a better explanation why we need it twice.

'7.1.0': doc => {
// [TSVB] Migrate percentile-rank aggregation (value -> values)
const migratePercentileRankAggregation = doc => {
Expand Down
88 changes: 88 additions & 0 deletions src/legacy/core_plugins/kibana/migrations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,94 @@ Object {
});

describe('visualization', () => {
describe('date histogram time zone removal', () => {
const migrate = doc => migrations.visualization['6.7.0'](doc);
let doc;
beforeEach(() => {
doc = {
attributes: {
visState: JSON.stringify({
aggs: [
{
'enabled': true,
'id': '1',
'params': {
// Doesn't make much sense but we want to test it's not removing it from anything else
time_zone: 'Europe/Berlin',
},
'schema': 'metric',
'type': 'count'
},
{
'enabled': true,
'id': '2',
'params': {
'customInterval': '2h',
'drop_partials': false,
'extended_bounds': {},
'field': 'timestamp',
'time_zone': 'Europe/Berlin',
'interval': 'auto',
'min_doc_count': 1,
'useNormalizedEsInterval': true
},
'schema': 'segment',
'type': 'date_histogram'
},
{
'enabled': true,
'id': '3',
'params': {
'customBucket': {
'enabled': true,
'id': '1-bucket',
'params': {
'customInterval': '2h',
'drop_partials': false,
'extended_bounds': {},
'field': 'timestamp',
'interval': 'auto',
'min_doc_count': 1,
'time_zone': 'Europe/Berlin',
'useNormalizedEsInterval': true
},
'type': 'date_histogram'
},
'customMetric': {
'enabled': true,
'id': '1-metric',
'params': {},
'type': 'count'
}
},
'schema': 'metric',
'type': 'max_bucket'
},
]
}),
}
};
});

it('should remove time_zone from date_histogram aggregations', () => {
const migratedDoc = migrate(doc);
const aggs = JSON.parse(migratedDoc.attributes.visState).aggs;
expect(aggs[1]).not.toHaveProperty('params.time_zone');
});

it('should not remove time_zone from non date_histogram aggregations', () => {
const migratedDoc = migrate(doc);
const aggs = JSON.parse(migratedDoc.attributes.visState).aggs;
expect(aggs[0]).toHaveProperty('params.time_zone');
});

it('should remove time_zone from nested aggregations', () => {
const migratedDoc = migrate(doc);
const aggs = JSON.parse(migratedDoc.attributes.visState).aggs;
expect(aggs[2]).not.toHaveProperty('params.customBucket.params.time_zone');
});
});

describe('7.0.0', () => {
const migrate = doc => migrations.visualization['7.0.0'](doc);
const generateDoc = ({ type, aggs }) => ({
Expand Down
7 changes: 7 additions & 0 deletions src/legacy/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 @@ -160,6 +160,13 @@ export const dateHistogramBucketAgg = new BucketAggType({
const isDefaultTimezone = config.isDefault('dateFormat:tz');
return isDefaultTimezone ? detectedTimezone || tzOffset : config.get('dateFormat:tz');
},
serialize() {
// We don't want to store the `time_zone` parameter ever in the saved object for the visualization.
// If we would store this changing the time zone in Kibana would not affect any already saved visualizations
// anymore, which is not the desired behavior. So always returning undefined here, makes sure we're never
// saving that parameter and just keep it "transient".
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, glad you explained this to minimize "WTF moments" in the future.

},
},
{
name: 'drop_partials',
Expand Down