[APM] Moving the date picker into APM

[jasonrhodes]

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.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

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

[jasonrhodes]

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.

[jasonrhodes]

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

[sorenlouv]

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.

[jasonrhodes]

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.

[sorenlouv]

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?

[sorenlouv]

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?

[jasonrhodes]

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]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[formgeist]

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]

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]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[jasonrhodes]

@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]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

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:

kibana/x-pack/plugins/apm/public/store/urlParams.ts

Line 61 in e6f3016

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.

[jasonrhodes]

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]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[jasonrhodes]

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

[sorenlouv]

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

[jasonrhodes]

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.

[sorenlouv]

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

[jasonrhodes]

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

[sorenlouv]

Looks good 👍

[sorenlouv]

@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]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

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.