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

[APM] Moving the date picker into APM #31311

Merged
merged 33 commits into from
Mar 7, 2019

Conversation

jasonrhodes
Copy link
Member

@jasonrhodes jasonrhodes commented Feb 15, 2019

Closes #30388
Closes #32025

This PR removes the angular date picker entirely and pulls in the EuiSuperDatePicker as a flex element to the right of the existing "kuery bar".

Here is how it looks without the refresh/update button, as mentioned by @formgeist in the comments below. As noted there, this results in more network requests as users interact with the absolute dates/times.

screen shot 2019-02-19 at 6 55 32 am

@jasonrhodes jasonrhodes requested a review from a team as a code owner February 15, 2019 20:02
@jasonrhodes jasonrhodes reopened this Feb 15, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

to: '',
isPaused: true,
refreshInterval: 0
};
Copy link
Member

@sorenlouv sorenlouv Feb 18, 2019

Choose a reason for hiding this comment

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

Why is this component stateful? Shouldn't it just read the props from the url (via urlParams where we can also set the defaults)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's possibly true, the state was just set up to get the EUI component to display correctly since it's controlled from here. But the query state may be enough to keep it synced, I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, while we're still using rison, we need to store the decoded values in state or else decode them in every render.

Copy link
Member

@sorenlouv sorenlouv Feb 19, 2019

Choose a reason for hiding this comment

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

Ah right, while we're still using rison, we need to store the decoded values in state or else decode them in every render.

Did you find any perf issues? Else it sounds a little like premature optimisation.
I think this decoding issue is yet another reason to get rid of Rison.
Even if we can't get rid of it, I think we should still keep the parsing of url params consistent with all the other url params - and then we can apply memoization where needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm ok. Not sure about premature optimization, it just seems a bit weird to do the decode in every render and also in every refresh cycle. I’ll wait until we make a final decision on rison to worry about this though.

I think we should still keep the parsing of url params consistent with all the other url params - and then we can apply memoization where needed.

Do you mean by storing them in the redux store? We don't currently store any of this local datepicker-specific info in redux -- urlParams.start and urlParams.end are the calculated date values used by all the ES queries, but these date values need to be the raw strings found in from and to so they can be passed into the date picker component that way. We don't use those raw strings anywhere else in the app, and we don't use the pause and refresh interval state anywhere else either, fwiw.

Once we sort the rison question I'll revisit this and see if we can remove the local state.

Copy link
Member

@sorenlouv sorenlouv Feb 19, 2019

Choose a reason for hiding this comment

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

We don't currently store any of this local datepicker-specific info in redux

No, but don't you think that's odd when we handle the other url params in the reducer?

Copy link
Member

Choose a reason for hiding this comment

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

Once we sort the rison question I'll revisit this and see if we can remove the local state

What do you think about trying it out, and seeing if it simplifies things, and then make a decision?

Copy link
Member Author

Choose a reason for hiding this comment

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

don't you think that's odd when we handle the other url params in the reducer?

I get what you're saying here re: consistency, but it also seems odd to move things into Redux that are only ever used in this one local component, and a bit complicated to manage urlParams.start next to urlParams.from etc. Not sure which odd thing is better here… let me think on it and we can chat in stand up.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@formgeist
Copy link
Contributor

Perhaps wrong place to drop this in, but as we addressed filtering (#30943), I'm thinking of setting showUpdateButton to false since we don't need it to take up prime real estate in that horizontal area. Would it be possible to include in this PR?

@jasonrhodes
Copy link
Member Author

Perhaps wrong place to drop this in, but as we addressed filtering (#30943), I'm thinking of setting showUpdateButton to false since we don't need it to take up prime real estate in that horizontal area. Would it be possible to include in this PR?

Yep, this is easy. To be clear, removing that button results in a lot more network requests as people interact with the dates and times, but if we're okay with that then this is just a single prop change on the picker.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

jasonrhodes commented Feb 19, 2019

@sqren I took a stab at some less-mocked style "integration" tests for this DatePicker component -> e82415b

Let me know what you think.

(This should make it a little easier for me to rip out the rison stuff and verify things still work the same way, if we go that way.)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

const converted = {
...query,
refreshPaused:
query.refreshPaused === 'true' || query.refreshPaused === true,
Copy link
Member

@sorenlouv sorenlouv Feb 20, 2019

Choose a reason for hiding this comment

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

Value will always be a string, right? Also, can you make a utility method for doing this, to make the intention clearer?

refreshPaused: toBoolean(query.refreshPaused)

This is one of the reasons I do all conversion in urlParams.ts eg:

page: toNumber(page) || 0,

The rest of the application only uses urlParams (and not the raw url values) and thus can be sure they are the correct type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I mean I'm not totally against using urlParams for all of this. I think it may cause a little confusion around start/end vs to/from, but if we think that's the simplest way to handle all these values I can probably get behind it. What do you think?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

TIL jest has fake timers ... that might simplify some of the tests here, gonna take a stab.

...APM_DEFAULT_TIME_OPTIONS,
...query
} as DatePickerParams;
});
Copy link
Member

@sorenlouv sorenlouv Feb 21, 2019

Choose a reason for hiding this comment

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

I had a little bit of a hard time understanding what this method was actually returning due to the conditionals and spreads. Since it's only four params I think it's worth making them explicit. What do you think about:

public getParamsFromSearch = (search: string) => {
  const { rangeFrom, rangeTo, refreshPaused, refreshInterval } = toQuery(
    search
  );
  return {
    rangeFrom: rangeFrom || 'now-24h',
    rangeTo: rangeTo || 'now',
    refreshPaused: toBoolean(refreshPaused),
    refreshInterval: toNumber(refreshInterval) || 0
  };
};

btw. This requires some minor modifications to APMQueryParams in url_helpers

Copy link
Member Author

@jasonrhodes jasonrhodes Feb 21, 2019

Choose a reason for hiding this comment

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

Hm, yeah, that function could use some work. Once I get the types sorted based on your good feedback there, I'll look at this again and see what we can do.

Just want to make sure we can default everything, but if we use refreshPaused = true with a default param in the destructure, I think it may still work.

Copy link
Member

Choose a reason for hiding this comment

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

ah, missed the default for refreshPaused. Maybe toBoolean should take a default value as a second param?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I was actually able to move all of these defaults up into the destructure, which reads nicely and avoids the confusion from before.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍

@sorenlouv
Copy link
Member

@jasonrhodes I've added a bunch of feedback regarding types. I got it to work without any type casting in a local branch, and can push that up if you'd like or we can zoom about it.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded


return qs.stringify(encoded, '&', '=', {
encodeURIComponent: (value: string) => value
});
Copy link
Member

@sorenlouv sorenlouv Mar 7, 2019

Choose a reason for hiding this comment

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

I'm not even sure if I like the follow proposal but at least it's super explicit (which I do like):

export function fromQuery(query: QueryParams) {
  const {
    detailTab,
    flyoutDetailTab,
    kuery,
    page,
    sortDirection,
    sortField,
    spanId,
    traceId,
    transactionId,
    waterfallItemId
  } = query;

  const search = [];

  if (detailTab) {
    search.push(`detailTab=${detailTab}`);
  }

  if (flyoutDetailTab) {
    search.push(`flyoutDetailTab=${flyoutDetailTab}`);
  }

  if (kuery) {
    search.push(`kuery=${encodeURIComponent(kuery)}`);
  }

  // ...

  return search.join('&');
}

This way it's very clear how each param is encoded.

@jasonrhodes jasonrhodes merged commit 8c7ca7f into elastic:master Mar 7, 2019
@jasonrhodes jasonrhodes deleted the move-date-picker/apm-30388 branch March 7, 2019 21:22
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Mar 7, 2019
* WIP moving the date picker into APM

* Stable version of EUI date picker controls, still needs layout

* Flex layout for kuery bar and date picker

* Removes angular time picker logic and layout

* Fixes snapshot test

* Adds integration tests for date picker

* Simplifies refresh cycle with setTimeout over requestAnimationFrame

* Removes rison and local state from APM date picker flow

* Adds refresh tests, fixes some refresh logic

* Moves temporary EUI types out of component

* Moves toBoolean helper and fixes TS error

* Removes unused Link import

* Types for datepicker (WIP)

* Updates default date picker values after merging in Søren's type changes

* Streamlines new APM query types to prevent duplication

* Uses jest fake timers for refresh tests

* Updates url handling to remove Rison from APM URLs, keeps Rison in outgoing Kibana links

* Move filter bar up and out from within a react-redux-request to avoid catch-22 circular dependency

* Separates rison encoding from regular url handling

* Sets start and end defaults in urlParams reducer

* Adds IUrlParams type to initial state with correct typing

* Updated rison-related snapshots

* Resolves failing tests related to query param management

* Adds more tests for Kibana Link and Kibana Rison Link components

* Re-enables the update button for the EUI super date picker

* Adds more Discover link tests

* Moved getRenderedHref to testHelpers, switched Discover Links integration tests to TS, and added ML link integration test

* Changes how getRenderedHref works to make it clearer where location state is coming from

* Updates obsolete snapshot

* Fixes typescript-discovered errors and type errors

* Finishes up url_helpers tests, removes dead commented tests

* Removes temporary date picker types from APM

* Fixes common case for an existing URL bug by not encoding range params
jasonrhodes added a commit that referenced this pull request Mar 8, 2019
* WIP moving the date picker into APM

* Stable version of EUI date picker controls, still needs layout

* Flex layout for kuery bar and date picker

* Removes angular time picker logic and layout

* Fixes snapshot test

* Adds integration tests for date picker

* Simplifies refresh cycle with setTimeout over requestAnimationFrame

* Removes rison and local state from APM date picker flow

* Adds refresh tests, fixes some refresh logic

* Moves temporary EUI types out of component

* Moves toBoolean helper and fixes TS error

* Removes unused Link import

* Types for datepicker (WIP)

* Updates default date picker values after merging in Søren's type changes

* Streamlines new APM query types to prevent duplication

* Uses jest fake timers for refresh tests

* Updates url handling to remove Rison from APM URLs, keeps Rison in outgoing Kibana links

* Move filter bar up and out from within a react-redux-request to avoid catch-22 circular dependency

* Separates rison encoding from regular url handling

* Sets start and end defaults in urlParams reducer

* Adds IUrlParams type to initial state with correct typing

* Updated rison-related snapshots

* Resolves failing tests related to query param management

* Adds more tests for Kibana Link and Kibana Rison Link components

* Re-enables the update button for the EUI super date picker

* Adds more Discover link tests

* Moved getRenderedHref to testHelpers, switched Discover Links integration tests to TS, and added ML link integration test

* Changes how getRenderedHref works to make it clearer where location state is coming from

* Updates obsolete snapshot

* Fixes typescript-discovered errors and type errors

* Finishes up url_helpers tests, removes dead commented tests

* Removes temporary date picker types from APM

* Fixes common case for an existing URL bug by not encoding range params
@sorenlouv sorenlouv mentioned this pull request Mar 8, 2019
7 tasks
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Mar 11, 2019
* WIP moving the date picker into APM

* Stable version of EUI date picker controls, still needs layout

* Flex layout for kuery bar and date picker

* Removes angular time picker logic and layout

* Fixes snapshot test

* Adds integration tests for date picker

* Simplifies refresh cycle with setTimeout over requestAnimationFrame

* Removes rison and local state from APM date picker flow

* Adds refresh tests, fixes some refresh logic

* Moves temporary EUI types out of component

* Moves toBoolean helper and fixes TS error

* Removes unused Link import

* Types for datepicker (WIP)

* Updates default date picker values after merging in Søren's type changes

* Streamlines new APM query types to prevent duplication

* Uses jest fake timers for refresh tests

* Updates url handling to remove Rison from APM URLs, keeps Rison in outgoing Kibana links

* Move filter bar up and out from within a react-redux-request to avoid catch-22 circular dependency

* Separates rison encoding from regular url handling

* Sets start and end defaults in urlParams reducer

* Adds IUrlParams type to initial state with correct typing

* Updated rison-related snapshots

* Resolves failing tests related to query param management

* Adds more tests for Kibana Link and Kibana Rison Link components

* Re-enables the update button for the EUI super date picker

* Adds more Discover link tests

* Moved getRenderedHref to testHelpers, switched Discover Links integration tests to TS, and added ML link integration test

* Changes how getRenderedHref works to make it clearer where location state is coming from

* Updates obsolete snapshot

* Fixes typescript-discovered errors and type errors

* Finishes up url_helpers tests, removes dead commented tests

* Removes temporary date picker types from APM

* Fixes common case for an existing URL bug by not encoding range params
jasonrhodes added a commit that referenced this pull request Mar 11, 2019
* [APM] Moving the date picker into APM (#31311)

* WIP moving the date picker into APM

* Stable version of EUI date picker controls, still needs layout

* Flex layout for kuery bar and date picker

* Removes angular time picker logic and layout

* Fixes snapshot test

* Adds integration tests for date picker

* Simplifies refresh cycle with setTimeout over requestAnimationFrame

* Removes rison and local state from APM date picker flow

* Adds refresh tests, fixes some refresh logic

* Moves temporary EUI types out of component

* Moves toBoolean helper and fixes TS error

* Removes unused Link import

* Types for datepicker (WIP)

* Updates default date picker values after merging in Søren's type changes

* Streamlines new APM query types to prevent duplication

* Uses jest fake timers for refresh tests

* Updates url handling to remove Rison from APM URLs, keeps Rison in outgoing Kibana links

* Move filter bar up and out from within a react-redux-request to avoid catch-22 circular dependency

* Separates rison encoding from regular url handling

* Sets start and end defaults in urlParams reducer

* Adds IUrlParams type to initial state with correct typing

* Updated rison-related snapshots

* Resolves failing tests related to query param management

* Adds more tests for Kibana Link and Kibana Rison Link components

* Re-enables the update button for the EUI super date picker

* Adds more Discover link tests

* Moved getRenderedHref to testHelpers, switched Discover Links integration tests to TS, and added ML link integration test

* Changes how getRenderedHref works to make it clearer where location state is coming from

* Updates obsolete snapshot

* Fixes typescript-discovered errors and type errors

* Finishes up url_helpers tests, removes dead commented tests

* Removes temporary date picker types from APM

* Fixes common case for an existing URL bug by not encoding range params

* Fixes x-pack tests

* Fixes type errors in x-pack
jasonrhodes added a commit that referenced this pull request Mar 11, 2019
* WIP moving the date picker into APM

* Stable version of EUI date picker controls, still needs layout

* Flex layout for kuery bar and date picker

* Removes angular time picker logic and layout

* Fixes snapshot test

* Adds integration tests for date picker

* Simplifies refresh cycle with setTimeout over requestAnimationFrame

* Removes rison and local state from APM date picker flow

* Adds refresh tests, fixes some refresh logic

* Moves temporary EUI types out of component

* Moves toBoolean helper and fixes TS error

* Removes unused Link import

* Types for datepicker (WIP)

* Updates default date picker values after merging in Søren's type changes

* Streamlines new APM query types to prevent duplication

* Uses jest fake timers for refresh tests

* Updates url handling to remove Rison from APM URLs, keeps Rison in outgoing Kibana links

* Move filter bar up and out from within a react-redux-request to avoid catch-22 circular dependency

* Separates rison encoding from regular url handling

* Sets start and end defaults in urlParams reducer

* Adds IUrlParams type to initial state with correct typing

* Updated rison-related snapshots

* Resolves failing tests related to query param management

* Adds more tests for Kibana Link and Kibana Rison Link components

* Re-enables the update button for the EUI super date picker

* Adds more Discover link tests

* Moved getRenderedHref to testHelpers, switched Discover Links integration tests to TS, and added ML link integration test

* Changes how getRenderedHref works to make it clearer where location state is coming from

* Updates obsolete snapshot

* Fixes typescript-discovered errors and type errors

* Finishes up url_helpers tests, removes dead commented tests

* Removes temporary date picker types from APM

* Fixes common case for an existing URL bug by not encoding range params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants