[lens] Calculate existence of fields in datasource

[wylieconlon]

[cchaos]

It feels weird to me to throw this option under the Filter by Type control. Can we brainstorm a different location? Maybe (for now) put a gear icon button on the right side of that whole search/filter line with a dropdown to start accumulating some of these settings?

[wylieconlon]

Sure, I can rearrange this a bit. Are you suggesting that the types filter and existence filter should both go into that gear icon? Otherwise, the search box could get too small. Maybe we can split it into multiple lines, or expand the search box while focused?

[elasticmachine]

💔 Build Failed

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

[wylieconlon]

@cchaos Another option is to change the text that says "Types" to say "Filters", which might be similar to changing to a gear icon.

[wylieconlon]

I just changed the text in the most recent one, but it's easy to make other layout changes:

[elasticmachine]

💔 Build Failed

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

[elasticmachine]

💚 Build Succeeded

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

[cchaos]

How about this configuration that separates the actual types list from the toggle.

[kertal]

Will continue reviewing next week, just a quick question, why do you need fieldFormats here?

[wylieconlon]

Good catch- I created this PR by splitting up a larger chunk of work into 2 pieces, and this is the second piece. In a follow-up PR we will have a popover to show histograms/terms of sampled field data, and that needs to be formatted

[wylieconlon]

@cchaos Thanks, I like that idea a lot. I've implemented it except the EuiSwitch label doesn't push the content width:

[elasticmachine]

💔 Build Failed

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

[chrisdavies]

LGTM w/ tweaks. I think there are a few places that could be cleaned up and optimized at the same time, and I really think we should be doing an HTTP get instead of a post. I don't think I'll need to re-review unless you want me to.

You put a lot of work into this, and it's definitely a big improvement in my opinion!

[chrisdavies]

Not a big deal, but I wonder if this /api/lens path should be in a const in our common folder?

[wylieconlon]

Good idea. Updated here and one other place.

[chrisdavies]

The async / await is unnecessary: server.events.on('stop', () => plugin.stop()); You can probably just do server.events.on('stop', plugin.stop);

[wylieconlon]

You're right, I'm going to make it all synchronous. I was going back and forth on this while developing because I think we will eventually want async here once we are fully on the new platform.

[chrisdavies]

This is fine, but I wonder if we should consider using the action / reducer pattern instead of setState as the core of our API... probably not? But it would get around a lot of our need for memoization and things like that.

[wylieconlon]

Going to follow up with you on this offline

[chrisdavies]

The double filter here seems unnecessary.

Also, there is an O(M * N) operation done every render (paginatedFields.length * currentIndexPattern.fields.length) in the "look up field by name" logic. This is also repeated further down. We should probably pull this out into a fieldByName dictionary up at the top of this function:

const currentIndexPattern = indexPatterns[currentIndexPatternId];
const allFields = currentIndexPattern.fields;
const fieldByName = _.indexBy(allFields, 'name');
const availableFieldTypes = uniq(allFields.map(({ type }) => type)).filter(
  type => type in fieldTypeNames
);
// etc...

So this filter block becomes something like this, maybe:

const isEmpty = (field: IndexPatternField) => {
  const indexField =
    currentIndexPattern &&
    currentIndexPattern.hasExistence &&
    fieldByName[field.name];

  return indexField && indexField.exists;
}

const displayedFields = allFields
  .filter(field =>
    (showEmptyFields || !isEmpty(field))
      && (localState.typeFilter.length === 0 || localState.typeFilter.includes(field.type as DataType))
      && (localState.nameFilter.length === 0 || field.name.toLowerCase().includes(localState.nameFilter.toLowerCase()))
      && (supportedFieldTypes.includes(field.type));
  });

[wylieconlon]

I've updated this to use a map instead of doing the lookup each time and to simplify the double filtering, but I did not fully change the filtering logic like you're suggesting.

[chrisdavies]

I think this should be a get, not a post, since it's fetching data, not modifying or creating it.

[wylieconlon]

Discussed offline. I'm concerned about the size of the body here, and given how large index pattern saved objects are, it's very likely that this would be rejected by browsers.

[chrisdavies]

I find hasExistence to be a bit confusing. It kind of sounds like it means: 'does this index pattern exist?' when what I think it means is: 'have we loaded field existence data for this index pattern?'

Maybe: hasFieldExistenceData? Dunno. Naming is hard.

Alternatively a), we could load field existence data along with the fields (as mentioned elsewhere) and this can go away.

Alternatively b), we could keep field existence data separate from field info, so this could go away, and we would instead check for the truthiness of the fieldExistence property:

fieldExistence: {
  foo: {
    exists: true;
    cardinality: 3,
    count: 390,
  },
}

[wylieconlon]

Like we discussed offline, I think we're going to want a custom response from the server that contains the existence information and other processing we want. If that's the case then we'll be able to delete this hasExistence name completely because we will always have it. And on a field level we will also always know whether the field exists. So I think these naming questions can be ignored for now, although I've added a TODO here

[chrisdavies]

Nice! Yeah. I think this is really helpful to have in a readme.

[wylieconlon]

Yeah, it was helpful when I had to update the functional test

[chrisdavies]

As mentioned elsewhere, I think this should be a get, and the params should be URL-encoded in some way. I know it's kind of a hassle vs just posting JSON, but I think it's worth sticking to HTTP semantics as much as possible, for firewall, security, caching, etc purposes.

[chrisdavies]

Should probably just use a set here: const expectedKeys = new Set(fields.map(f => f.name));

And on line 128: if (!expectedKeys.has(field.name)) { or a filter.

[wylieconlon]

Done

[chrisdavies]

I think this could be simplified to:

const matches = docs.map(doc => {
  if (!doc) {
    return;
  }

  return get(doc._source, field.parent || field.name);
});

In fact this entire block could probably be simplified:

indexPattern.forEach(field => {
  if (!expectedKeys.has(field.name)) {
    return;
  }

  docs.forEach(doc => {
    if (!doc) {
      return;
    }

    const match = get(doc._source, field.parent || field.name);
    const record = overallKeys[field.name];
    if (record) {
      record.count += 1;
      record.samples.add(match);
    } else {
      overallKeys[field.name] = {
        count: 1,
        // Using a set here makes the most sense and avoids the later uniq computation
        samples: new Set(match),
      };
    }
  });
});

return _.mapValues(overallKeys, record => ({
  count: record.count,
  cardinality: record.samples.size,
}));

[wylieconlon]

Good idea using a Set- the only change I had to make here is to the return type, which I had set up as Record<string, { count, cardinality }> and is an array here.

[wylieconlon]

Haven't responded to some of your larger comments, but will discuss offline to see if we can resolve them

[wylieconlon]

You're right, I'm going to make it all synchronous. I was going back and forth on this while developing because I think we will eventually want async here once we are fully on the new platform.

[wylieconlon]

Good idea. Updated here and one other place.

[wylieconlon]

Done

[wylieconlon]

Yeah, it was helpful when I had to update the functional test

[wylieconlon]

Going to follow up with you on this offline

[wylieconlon]

Moved

[wylieconlon]

Agreed

[wylieconlon]

With this current setup, this request is run once per indexpattern per session, like one of your other comments. Toggling the checkbox with this setup requires no network requests

[elasticmachine]

💔 Build Failed

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

[flash1293]

It feels a bit strange to have the setting in the data panel take effect here. If a user is in the popover editor and wants to select a fields they know exists but the existence filter is on, it won't show up and the user has to close the editor, go over to the data panel and deactivate the filter there. It gets more confusing because the type filters (string, number, date) in the data panel don't take effect here. Maybe we should just put them down to the end of the list?

[wylieconlon]

I changed this based on early feedback that it was confusing to have two ways of doing this. I feel pretty strongly that we have a good default- hiding "empty" fields is going to help in a lot of common cases. I don't intend to change this right away, but if we get more feedback about this it's easy to add more control

[flash1293]

nit exists and missing look like modifiers which are normally separated by double dash: lnsFieldListPanel__field--${exists ? 'exists' : 'missing'}

[flash1293]

Thanks for extensive tests for this!

[elasticmachine]

💔 Build Failed

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

[elasticmachine]

💚 Build Succeeded

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