Skip to content

[data views] composite runtime fields in api layer#125183

Merged
mattkime merged 42 commits intoelastic:mainfrom
mattkime:composite_runtime_field_data_view
Feb 24, 2022
Merged

[data views] composite runtime fields in api layer#125183
mattkime merged 42 commits intoelastic:mainfrom
mattkime:composite_runtime_field_data_view

Conversation

@mattkime
Copy link
Contributor

@mattkime mattkime commented Feb 10, 2022

Summary

Add composite runtime fields to the data view api layer. Broken out of #110223

Part of phase 3 of https://github.com/elastic/kibana-team/issues/307

Composite runtime field ES docs - https://www.elastic.co/guide/en/elasticsearch/reference/current/runtime-examples.html#runtime-examples-grok-composite


To test composite runtime field, provide data view id -

curl -u elastic -X POST "localhost:5601/api/data_views/data_view/${id}/runtime_field" -H 'kbn-xsrf: true' -H 'Content-Type: application/json' -d @"composite_runtime.txt"

composite_runtime.txt content -

{
  "name": "composite_runtime",
  "runtimeField": {
    "type": "composite",
    "script": {
      "source": "emit(\"a\",\"a\"); emit(\"b\",\"b\")"
    },
    "fields": {
      "a": {
        "type": "keyword"
      },
      "b": {
        "type": "keyword"
      }
    }
  }
}

and delete

curl -u elastic -X DELETE "localhost:5601/api/data_views/data_view/${id}/runtime_field/composite_runtime" -H 'kbn-xsrf: true' -H 'Content-Type: application/json'

Testing

  • Use API to submit composite runtime field
  • Verify fields are in data view management.
  • Verify they cannot be deleted
  • Verify they can be edited like mapped fields - can't change name or type, no set script ability
  • Verify regular runtime fields work as expected
  • Go to discover and verify that runtime fields work for search and query as expected
  • Remove composite runtime field via API
  • verify that all child fields have been removed.

@mattkime mattkime changed the title composite runtime fields in data view layer [data views] composite runtime fields in api layer Feb 10, 2022
);
});

it('returns error when attempting to fetch a field which is not a runtime field', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this test is basically the same as the one on line 54.

expect(response1.status).to.be(404);
});

it('returns error when attempting to delete a field which is not a runtime field', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as test above, I don't see a need for a separate error code when removing a non runtime field

@mattkime mattkime marked this pull request as ready for review February 10, 2022 03:59
@mattkime mattkime requested review from a team as code owners February 10, 2022 03:59
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Runtime Fields v8.2.0 release_note:skip Skip the PR/issue when compiling release notes Team:AppServicesSv labels Feb 10, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@mattkime mattkime requested a review from Dosant February 10, 2022 04:00
@mattkime mattkime requested a review from a team as a code owner February 10, 2022 04:27
@flash1293
Copy link
Contributor

flash1293 commented Feb 10, 2022

@mattkime The description of this PR doesn't really explain what's going on and the linked PR is pretty bare as well. Could you elaborate a bit on the context of these changes and why they are necessary? I want to make sure the changes done won't break anything else in VisEditors code as it seems like they are not backwards compatible in some ways.

@mattkime
Copy link
Contributor Author

@flash1293 Sorry for that, I've updated the description with additional context.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

VisEditors changes LGTM

Copy link
Contributor

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml LGTM

@mattkime
Copy link
Contributor Author

@elasticmachine merge upstream

@mattkime
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but it seems that previous feedback wasn't addressed or wasn't communicated that it won't be addressed:

  1. I retested, and this seems wasn't fixed? #125183 (comment) is it intentional? From your response, I thought you are planning to fix this

  2. Hm, I think this one also wasn't fixed / no response. still can reproduce.
    #125183 (comment)

3 I also think this is a fair suggestion, but this was missed

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra plugin changes look good. thanks for improving the DataView API!

@mattkime
Copy link
Contributor Author

mattkime commented Feb 23, 2022

@Dosant

  1. I took another look at this and it appears to be working for me. I should have addressed your comment about cleaning up fieldAttrs - we never clean them up. This payload correctly returned fieldAttrs and the value was correctly set in data views management -
{
  "name": "composite_runtime",
  "runtimeField": {
    "type": "composite",
    "script": {
      "source": "emit(\"a\",\"a\"); emit(\"b\",\"b\")"
    },
    "fields": {
      "a": {
        "type": "keyword",
        "popularity": 100
      },
      "b": {
        "type": "keyword"
      }
    }
  }
}
  1. I'm not sure how to do this. Do you know how? It seems like a case where we want an 'or' statement in our schema.

  2. Just addressed this.

@Dosant
Copy link
Contributor

Dosant commented Feb 23, 2022

@mattkime,

I took another look at this and it appears to be working for me. I should have addressed your comment about cleaning up fieldAttrs - we never clean them up. This payload correctly returned fieldAttrs and the value was correctly set in data views management -

If you say that this is expected that we don't clean up fieldAttrs after a field is deleted then sounds good

I'm not sure how to do this. Do you know how? It seems like a case where we want an 'or' statement in our schema.

Maybe simple through oneOf? Then we can also be specific about runtime field type:
if type is composite then you get one schema, otherwise another schema

export const runtimeFieldSpec = schema.oneOf([
  schema.object({
    type: runtimeFieldNonCompositeFieldsSpecTypeSchema,
    script: schema.maybe(
      schema.object({
        source: schema.string(),
      })
    ),
    format: schema.maybe(serializedFieldFormatSchema),
    customLabel: schema.maybe(schema.string()),
    popularity: schema.maybe(
      schema.number({
        min: 0,
      })
    ),
  }),
  schema.object({
    type: runtimeFieldSpecTypeSchema,
    script: schema.maybe(
      schema.object({
        source: schema.string(),
      })
    ),
    fields: schema.maybe(
      schema.recordOf(
        schema.string(),
        schema.object({
          type: runtimeFieldCompositeFieldsTypeSchema,
          format: schema.maybe(serializedFieldFormatSchema),
          customLabel: schema.maybe(schema.string()),
          popularity: schema.maybe(
            schema.number({
              min: 0,
            })
          ),
        })
      )
    ),
  }),
]);

If that won't work, then simply runtime checks inside API route handlers?

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM + previous suggestions

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataViews 43 44 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2784 2790 +6
dataViews 597 609 +12
total +18

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataViewManagement 119.8KB 119.9KB +89.0B
dataVisualizer 539.8KB 539.8KB -26.0B
visTypeVega 2.0MB 2.0MB -13.0B
total +50.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
dataViews 7 10 +3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViewFieldEditor 24.4KB 24.4KB +35.0B
dataViews 37.9KB 40.0KB +2.1KB
infra 89.1KB 89.0KB -171.0B
total +1.9KB
Unknown metric groups

API count

id before after diff
data 3378 3387 +9
dataViews 742 758 +16
total +25

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mattkime mattkime merged commit ffe964f into elastic:main Feb 24, 2022
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125183 or prevent reminders by adding the backport:skip label.

@mattkime mattkime added the backport:skip This PR does not require backporting label Feb 28, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 2, 2022
* composite runtime fields in data view layer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Runtime Fields release_note:skip Skip the PR/issue when compiling release notes v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.