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

Add support for multi-level self-nestings #1459

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented Jun 15, 2021

Add support to selectively allow a self-nesting to be reused inside another self-nesting.

For example, I want to nest the process.* fields within another nesting of process, process.target.*.

This type of reuse will need to be defined explicitly in the field definitions:

- name: process
  reusable:
    expected:
      - at: process
        as: target
      - at: process.target
        as: parent

Closes #1458

@ebeahan ebeahan self-assigned this Jun 15, 2021
@ebeahan ebeahan added the 1.11.0 label Jun 15, 2021
@ebeahan ebeahan requested a review from a team June 15, 2021 16:49
Copy link
Contributor

@kgeller kgeller left a comment

Choose a reason for hiding this comment

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

LGTM!

@djptek
Copy link
Contributor

djptek commented Jun 16, 2021

Do we really want this? I took a better look at the RFC 0016 & I understand the context, however, I don't agree nesting is the right way to go, principally because that may create technical debt downstream for how to unpack this, e.g- O11y. Adding comments to that RFC, sorry for coming in late on this one

@djptek
Copy link
Contributor

djptek commented Jun 16, 2021

There is an alternative to multi-level nesting - split to separate docs and flatten one level e.g. something like:

PUT test/_doc/001
{ "my_id": "001" }

PUT test/_doc/002
{ "my_id": "002", "my_parent_ids": "001" }

PUT test/_doc/003
{ "my_id": "003", "my_parent_ids": ["001", "002"]}

that would require multiple queries to assemble the full chain of parents but that is easier to code and safer than stacking them internally: consider this use case - a process that calls itself perpetually (that happens) may overflow the stack (possibly deliberately, in order to harvest data), either in ECS or worse, downstream :/

@ebeahan
Copy link
Member Author

ebeahan commented Jun 16, 2021

I framed this PR in the context of #1297, but let's discuss the changes here independently as a feature of tooling and the schema DSL, and if we want this ability to be available for use at the user's discretion.

Some additional background points:

  • Foreign reuses already allow a similar pattern by way of the order attribute. For example, group is reused under user, and then user is reused to create user.target.group.*.
  • There's been past discussion about the endpoint leveraging this type of pattern in the endpoint's schema.
  • Right now, this functionality is implied by the tool's docs and DSL, but it doesn't work as expected. I opt to leave the decision whether to self-nest up to the user but have the functionality available if they choose to use it.

I think we move forward with adding the functionality. If we adopt the pattern into ECS, we'll already have support in our tooling, but if we don't, the tooling will support it if a user chooses to do so.

@djptek
Copy link
Contributor

djptek commented Jun 17, 2021

Thanks for the additional clarifications @ebeahan

There's one comment on #1297 that I think is Key

we will need to make sure that we articulate what each reuse means

So provided each nesting is a manual design decision with peer review and not a consequence of a recursive algorithm, I'm good to go with this 👍

@djptek djptek self-requested a review June 17, 2021 07:53
Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

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

provided each nesting is a manual design decision with peer review and not a consequence of a recursive algorithm, LGTM 👍

@ebeahan ebeahan merged commit 73fb403 into elastic:master Jun 21, 2021
@ebeahan ebeahan deleted the multi-level-self-nesting branch June 21, 2021 18:59
ebeahan added a commit to ebeahan/ecs that referenced this pull request Jun 21, 2021
* support multi-level self-nesting

* add multi-level self-nesting unit testing

* add changelog entry
ebeahan added a commit that referenced this pull request Jun 21, 2021
* support multi-level self-nesting

* add multi-level self-nesting unit testing

* add changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-level self-nesting support
3 participants