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.

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

avoid day long gaps in sample data #20897

Merged
merged 9 commits into from Jul 22, 2018

Conversation

Projects
None yet
4 participants
@nreese
Copy link
Contributor

nreese commented Jul 17, 2018

fixes #20807

There was a bug in the sample data timestamp adjustment logic that allowed for a days long gap in the data. The bug was that partial weeks were not accounted for resulting in bugs like: this Friday and last Friday both getting adjusted to this Friday.

This PR cleans up the timestamp adjustment logic to acount for partial weeks. The PR also ensures consistency with Date methods ensuring that only methods that return values in local time are used.

Below is an example of what the data gap used to look like in Discover
screen shot 2018-07-17 at 9 36 52 am

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 17, 2018

@nreese nreese requested review from chrisdavies and stacey-gammon Jul 17, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 17, 2018

@nreese nreese requested review from cjcenizal and removed request for stacey-gammon Jul 18, 2018

@markov00
Copy link
Member

markov00 left a comment

Code LGTM.
I've tested the cases where tests fails using different timezone as specified #20020 (review) all tests passes.

@cjcenizal
Copy link
Contributor

cjcenizal left a comment

👍 Tested locally and this fixes the bug!

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jul 20, 2018

@cjcenizal Thanks for walking me through a much simpler way of translating the times. Unfortunately, the refactor was very large. Would you mind giving this another review?

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 20, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jul 20, 2018

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 20, 2018

@cjcenizal
Copy link
Contributor

cjcenizal left a comment

🔥 🔥 🔥 This reads beautifully. Great work man.

I left a couple minor suggestions in the code.

I tested locally and I don't see the gaps, though now it looks like there's less data? Does this look right to you? This is viewing the last 7 days in Discover:

image

return new Date(year, month, date);
}

export function dateToISO8601IgnoringTime(date) {

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Jul 20, 2018

Contributor

Minor nit, but personally I think the only logical and consistent way to treat acronyms is to camel case them like any other word. So this would be dateToIso8601IgnoringTime which would case it consistently with iso8601ToDateIgnoringTime.


// Translate source timestamp by targetReference timestamp,
// perserving the distance between source and sourceReference
export function translateTimeRelativeToDifference(source, sourceReference, targetReference) {

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Jul 20, 2018

Contributor

I renamed a variable here to help me reason about this code in the correct terms... it's a small change but maybe it will help others too?

export function translateTimeRelativeToDifference(source, sourceReference, targetReference) {
  const sourceDate = iso8601ToDateIgnoringTime(source);
  const sourceReferenceDate = iso8601ToDateIgnoringTime(sourceReference);
  const targetReferenceDate = iso8601ToDateIgnoringTime(targetReference);

  const timeDelta = sourceDate.getTime() - sourceReferenceDate.getTime();
  const translatedDate = (new Date(targetReferenceDate.getTime() + timeDelta));

  return `${dateToISO8601IgnoringTime(translatedDate)}T${source.substring(11)}`;
}

// Translate source timestamp by targetReference timestamp,
// perserving the week distance between source and sourceReference and day of week of the source timestamp
export function translateTimeRelativeToWeek(source, sourceReference, targetReference) {

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Jul 20, 2018

Contributor

I also added some comments here and changed some names to help me reason about this. Might be useful to others too.

export function translateTimeRelativeToWeek(source, sourceReference, targetReference) {
  const sourceReferenceDate = iso8601ToDateIgnoringTime(sourceReference);
  const targetReferenceDate = iso8601ToDateIgnoringTime(targetReference);

  // If these dates were in the same week, how many days apart would they be?
  const dayOfWeekDelta = sourceReferenceDate.getDay() - targetReferenceDate.getDay();

  // If we pretend that the targetReference is actually the same day of the week as the
  // sourceReference, then we can translate the source to the target while preserving their
  // days of the week.
  const normalizationDelta = dayOfWeekDelta * MILLISECONDS_IN_DAY;
  const normalizedTargetReference =
    dateToISO8601IgnoringTime(new Date(targetReferenceDate.getTime() + normalizationDelta));

  return translateTimeRelativeToDifference(
    source,
    sourceReference,
    normalizedTargetReference);
}
@cjcenizal

This comment has been minimized.

Copy link
Contributor

cjcenizal commented Jul 20, 2018

Ah nevermind I was looking at the wrong index pattern. Here's what I get using the sample data:

image

LGTM!

nreese added some commits Jul 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 20, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jul 20, 2018

Looks like flaky test

fail: "visualize app visualize lab mode "after all" hook"
22:31:49    │ proc  [ftr]      │        tryForTime timeout: Error: tryForTime timeout: [POST http://localhost:9515/session/c3a7ee92be6a5923bd6fdd21ae4eef98/element / {"using":"css selector","value":"[data-test-subj~=\"advancedSetting-resetField-visualize:enableLabs\"]"}] no such element: Unable to locate element: {"method":"css selector","selector":"[data-test-subj~="advancedSetting-resetField-visualize:enableLabs"]"}

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 22, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 22, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jul 22, 2018

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 22, 2018

@nreese nreese merged commit b319b5a into elastic:master Jul 22, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

nreese added a commit to nreese/kibana that referenced this pull request Jul 22, 2018

avoid day long gaps in sample data (elastic#20897)
* avoid day long gaps in sample data

* avoid using toISOString to avoid an timezone problems

* unskip sample test now that problem is fixed

* use much better cj algorithm for translating time

* cjcenizal review updates

* update funtion name in install.js

* push source reference date back a week

nreese added a commit that referenced this pull request Jul 23, 2018

avoid day long gaps in sample data (#20897) (#21070)
* avoid day long gaps in sample data

* avoid using toISOString to avoid an timezone problems

* unskip sample test now that problem is fixed

* use much better cj algorithm for translating time

* cjcenizal review updates

* update funtion name in install.js

* push source reference date back a week
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.