Skip to content

[Streams] Schema Editor advanced field mapping options#210667

Merged
Kerry350 merged 9 commits intoelastic:mainfrom
Kerry350:88-advanced-field-mapping-options
Feb 14, 2025
Merged

[Streams] Schema Editor advanced field mapping options#210667
Kerry350 merged 9 commits intoelastic:mainfrom
Kerry350:88-advanced-field-mapping-options

Conversation

@Kerry350
Copy link
Copy Markdown
Contributor

@Kerry350 Kerry350 commented Feb 11, 2025

Summary

Closes https://github.com/elastic/streams-program/issues/88

Adds JSON advanced field mapping parameters to the Schema Editor.

Main questions here are around the types and data structure. In this PR these are added as an additionalProperties property, but we may also want to have all of these parameters top level (like type and format). This version makes separating concerns easier in the UI and separating "first class" options vs advanced options, I could see pros and cons to both, and also things might be "upgraded" from advanced to first class later on. Also an open question on whether the MappingProperty type needs to be explicitly redefined for Zod (ES will obviously reject anything that isn't supported here).

Screenshot 2025-02-12 at 13 01 35

json_params

@Kerry350 Kerry350 added release_note:skip Skip the PR/issue when compiling release notes backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project v9.1.0 v8.19.0 labels Feb 11, 2025
@Kerry350 Kerry350 self-assigned this Feb 11, 2025
@Kerry350 Kerry350 marked this pull request as ready for review February 12, 2025 13:13
@Kerry350 Kerry350 requested a review from a team as a code owner February 12, 2025 13:13
@tonyghiani tonyghiani self-requested a review February 13, 2025 10:59
Copy link
Copy Markdown
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

Overall looks good, I left some minor notes on the code.

Regarding the additionalProperties vs top-level props, I like the separation of concerns as well, but this introduce potentially a de-sync between the stream and the component template.

For instance, a type defined in the additional properties (copy/paste, human error...) would override the defined type from the selector on the component template, and the user might never notice since the accordion is closed and the stored type in the stream remain the selector one.

As per following screenshot, the stream field would be:
{
    "type": "ip",
    "additionalProperties": {
        "type": "keyword"
    }
}

while in the component template it will be
{
  "host.ip": {
    "type": "keyword"
  }
}
Screenshot 2025-02-13 at 13 09 24

It might be worth excluding the type from the additionalProperties, wdyt?

A last note: when the additional properties have an unacceptable property, the error report is not very helpful, although we get the ES error message in the response, we can we pass it correctly into the error toast for a better feedback.
Screenshot 2025-02-13 at 12 41 08

@Kerry350
Copy link
Copy Markdown
Contributor Author

@tonyghiani

Regarding the additionalProperties vs top-level props, I like the separation of concerns as well, but this introduce potentially a de-sync between the stream and the component template.
For instance, a type defined in the additional properties (copy/paste, human error...) would override the defined type from the selector on the component template, and the user might never notice since the accordion is closed and the stored type in the stream remain the selector one.

Yeah, I'd considered this and ultimately thought if someone had explicitly defined something in advanced settings, then it would be okay to override things, but I guess it does cause a divergence in the source of truth for both type and format. I think what I'll do is move them all top level, and then add some functions to the schema package to "get advanced parameters" which will strip our first class properties.

A last note: when the additional properties have an unacceptable property, the error report is not very helpful, although we get the ES error message in the response, we can we pass it correctly into the error toast for a better feedback.

Yeah, that's fair. I think I went down this rabbit hole once before and it was to do with the way streamsRepositoryClient propagates it's error and the information passed on. I'll take a look again.

@tonyghiani
Copy link
Copy Markdown
Contributor

Yeah, I'd considered this and ultimately thought if someone had explicitly defined something in advanced settings, then it would be okay to override things, but I guess it does cause a divergence in the source of truth for both type and format. I think what I'll do is move them all top level, and then add some functions to the schema package to "get advanced parameters" which will strip our first class properties.

Sounds like a good approach to me 👌

Yeah, that's fair. I think I went down this rabbit hole once before and it was to do with the way streamsRepositoryClient propagates it's error and the information passed on. I'll take a look again.

It might help in this case too, I used something like this to get the error correctly from an ES propagated one:

} catch (error) {
      toasts.addError(new Error(error.body.message), {
        title: i18n.translate(
          'xpack.streams.streamDetailView.managementTab.enrichment.saveChangesError',
          { defaultMessage: "An issue occurred saving processors' changes." }
        ),
        toastMessage: error.body.message,
      });
    }

@Kerry350
Copy link
Copy Markdown
Contributor Author

@tonyghiani Ready for another look. I've made all of the parameter handling top level, and then pushed the stripping to getAdvancedParameters.

Copy link
Copy Markdown
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
streamsApp 313 315 +2

Async chunks

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

id before after diff
streamsApp 304.0KB 305.8KB +1.8KB

History

cc @Kerry350

@Kerry350 Kerry350 merged commit c9dfe9a into elastic:main Feb 14, 2025
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13330592124

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 14, 2025
## Summary

Closes elastic/streams-program#88

Adds JSON advanced field mapping parameters to the Schema Editor.

Main questions here are around the types and data structure. In this PR
these are added as an `additionalProperties` property, but we may also
want to have all of these parameters top level (like `type` and
`format`). This version makes separating concerns easier in the UI and
separating "first class" options vs advanced options, I could see pros
and cons to both, and also things might be "upgraded" from advanced to
first class later on. Also an open question on whether the
`MappingProperty` type needs to be explicitly redefined for Zod (ES will
obviously reject anything that isn't supported here).

![Screenshot 2025-02-12 at 13 01
35](https://github.com/user-attachments/assets/7082fed7-f445-476f-abb7-8f41d693d378)

![json_params](https://github.com/user-attachments/assets/521df9bf-cbd4-468a-9385-5787cdd5f906)

(cherry picked from commit c9dfe9a)
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 17, 2025
#211220)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Streams] Schema Editor advanced field mapping options
(#210667)](#210667)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kerry
Gallagher","email":"kerry.gallagher@elastic.co"},"sourceCommit":{"committedDate":"2025-02-14T13:52:18Z","message":"[Streams]
Schema Editor advanced field mapping options (#210667)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/streams-program/issues/88\r\n\r\nAdds JSON
advanced field mapping parameters to the Schema Editor.\r\n\r\nMain
questions here are around the types and data structure. In this
PR\r\nthese are added as an `additionalProperties` property, but we may
also\r\nwant to have all of these parameters top level (like `type`
and\r\n`format`). This version makes separating concerns easier in the
UI and\r\nseparating \"first class\" options vs advanced options, I
could see pros\r\nand cons to both, and also things might be
\"upgraded\" from advanced to\r\nfirst class later on. Also an open
question on whether the\r\n`MappingProperty` type needs to be explicitly
redefined for Zod (ES will\r\nobviously reject anything that isn't
supported here).\r\n\r\n![Screenshot 2025-02-12 at 13
01\r\n35](https://github.com/user-attachments/assets/7082fed7-f445-476f-abb7-8f41d693d378)\r\n\r\n\r\n![json_params](https://github.com/user-attachments/assets/521df9bf-cbd4-468a-9385-5787cdd5f906)","sha":"c9dfe9aab1ac2146f3c6ec3810ad35f8b753e6c3","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","Feature:Streams","v9.1.0","v8.19.0"],"title":"[Streams]
Schema Editor advanced field mapping
options","number":210667,"url":"https://github.com/elastic/kibana/pull/210667","mergeCommit":{"message":"[Streams]
Schema Editor advanced field mapping options (#210667)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/streams-program/issues/88\r\n\r\nAdds JSON
advanced field mapping parameters to the Schema Editor.\r\n\r\nMain
questions here are around the types and data structure. In this
PR\r\nthese are added as an `additionalProperties` property, but we may
also\r\nwant to have all of these parameters top level (like `type`
and\r\n`format`). This version makes separating concerns easier in the
UI and\r\nseparating \"first class\" options vs advanced options, I
could see pros\r\nand cons to both, and also things might be
\"upgraded\" from advanced to\r\nfirst class later on. Also an open
question on whether the\r\n`MappingProperty` type needs to be explicitly
redefined for Zod (ES will\r\nobviously reject anything that isn't
supported here).\r\n\r\n![Screenshot 2025-02-12 at 13
01\r\n35](https://github.com/user-attachments/assets/7082fed7-f445-476f-abb7-8f41d693d378)\r\n\r\n\r\n![json_params](https://github.com/user-attachments/assets/521df9bf-cbd4-468a-9385-5787cdd5f906)","sha":"c9dfe9aab1ac2146f3c6ec3810ad35f8b753e6c3"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/210667","number":210667,"mergeCommit":{"message":"[Streams]
Schema Editor advanced field mapping options (#210667)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/streams-program/issues/88\r\n\r\nAdds JSON
advanced field mapping parameters to the Schema Editor.\r\n\r\nMain
questions here are around the types and data structure. In this
PR\r\nthese are added as an `additionalProperties` property, but we may
also\r\nwant to have all of these parameters top level (like `type`
and\r\n`format`). This version makes separating concerns easier in the
UI and\r\nseparating \"first class\" options vs advanced options, I
could see pros\r\nand cons to both, and also things might be
\"upgraded\" from advanced to\r\nfirst class later on. Also an open
question on whether the\r\n`MappingProperty` type needs to be explicitly
redefined for Zod (ES will\r\nobviously reject anything that isn't
supported here).\r\n\r\n![Screenshot 2025-02-12 at 13
01\r\n35](https://github.com/user-attachments/assets/7082fed7-f445-476f-abb7-8f41d693d378)\r\n\r\n\r\n![json_params](https://github.com/user-attachments/assets/521df9bf-cbd4-468a-9385-5787cdd5f906)","sha":"c9dfe9aab1ac2146f3c6ec3810ad35f8b753e6c3"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Kerry Gallagher <kerry.gallagher@elastic.co>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Mar 22, 2025
## Summary

Closes elastic/streams-program#88

Adds JSON advanced field mapping parameters to the Schema Editor.

Main questions here are around the types and data structure. In this PR
these are added as an `additionalProperties` property, but we may also
want to have all of these parameters top level (like `type` and
`format`). This version makes separating concerns easier in the UI and
separating "first class" options vs advanced options, I could see pros
and cons to both, and also things might be "upgraded" from advanced to
first class later on. Also an open question on whether the
`MappingProperty` type needs to be explicitly redefined for Zod (ES will
obviously reject anything that isn't supported here).

![Screenshot 2025-02-12 at 13 01
35](https://github.com/user-attachments/assets/7082fed7-f445-476f-abb7-8f41d693d378)


![json_params](https://github.com/user-attachments/assets/521df9bf-cbd4-468a-9385-5787cdd5f906)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants