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

[RFC] 0017 Remove log.original stage 1 #1314

Merged
merged 6 commits into from
Apr 7, 2021
Merged

[RFC] 0017 Remove log.original stage 1 #1314

merged 6 commits into from
Apr 7, 2021

Conversation

djptek
Copy link
Contributor

@djptek djptek commented Mar 24, 2021

  • Have you signed the contributor license agreement? Y
  • Have you followed the contributor guidelines? Y
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process? Y
  • If submitting code/script changes, have you verified all tests pass locally using make test? n/a stage 1
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? n/a stage 1
  • Is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed. Y
  • Have you added an entry to the [CHANGELOG.next.md] (https://github.com/elastic/ecs/blob/master/CHANGELOG.next.md)? n/a stage 1

@djptek djptek changed the title rfc 0017 stage 1 [RFC] 0017 stage 1 Mar 24, 2021
@djptek djptek requested review from ebeahan and kgeller March 24, 2021 17:19
kgeller
kgeller previously approved these changes Mar 24, 2021
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this, @djptek!

I think overall it's shaping up well. Couple of "virtual paperwork" items 😸 :

  • Would you add the URL for this PR to the section towards the bottom for stage 1?
  • Also would you add my handle, @ebeahan, as the sponsor under the People section?

@@ -16,16 +16,18 @@ The removal of `log.original` will be considered a breaking change since the fie

| Field Set | Field(s) |
| --------- | -------- |
| [`log`](0000/log.yml) | `log.original` |
| [`event`](0000/event.yml) | `event.original` |
| [`log`](0017/log.yml) | `log.original` |
Copy link
Member

Choose a reason for hiding this comment

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

Including field definition files when adding or changing fields has proven useful in a couple of ways:

  • The fields can be used either with the --include flag to test out the field changes
  • Sometimes, the proposed field definitions get a little long, especially for brand new fieldsets. Separating the field definitions from the markdown keeps everything a bit tidier.

The way that custom files are merged with the current ECS fields, even if you remove a current ECS field it'll still be present in all the outputs. So it's not a problem to include these field definitions here, but unfortunately, we can't use them with the --include flag and get the expected result.

I do think we could consider having a consistent way to mark a field as deprecated in the ECS docs until we're ready to entirely remove the field. Looking at the Elastic docs repo, there is an asciidoc admonition available for deprecated that we might be able to leverage somehow: https://github.com/elastic/docs#old-section.

We have something similar for the beta attribute: https://github.com/elastic/ecs/blob/master/scripts/templates/field_details.j2#L13-L18

rfcs/text/0017-remove-log-original.md Show resolved Hide resolved
rfcs/text/0017-remove-log-original.md Outdated Show resolved Hide resolved
added URL for Stage 1 PR
added @ebeahan as sponsor
correct notes on extended description of `event.original` thanks for spotting @ebeahan
@epixa epixa changed the title [RFC] 0017 stage 1 [RFC] 0017 Remove log.original stage 1 Apr 5, 2021
@ebeahan
Copy link
Member

ebeahan commented Apr 5, 2021

Thanks, @djptek, for the updates! Everything is looking great here.

I'm going to list out the criteria for advancing to stage one. The one remaining item is capturing some of the potential implementation challenges and concerns in the proposal doc.

  • Opened pull request for this proposal revising the existing strawperson
  • Identified "sponsor" at Elastic who will participate in RFC process and take ownership of the change after the process completes
  • Outlined initial field definitions
  • High-level description of examples of usage
  • High-level description of example sources of data
  • Identified potential concerns and implementation challenges/complexity
    • Let's note under Scope of impact that Beats modules and Agent integration packages would be required to migrate if this change is adopted as proposed. No need to find or list every instance, but we should call out that those ingestion sources will be affected if this change takes place.
    • I think lines 10-13 noting the breaking changes could go under the Scope of impact section as well.
  • Subject matter experts identified and weighed in on the high level utility of these changes in the pull request
  • ECS team weighed in on appropriateness of these changes in the pull request

@djptek
Copy link
Contributor Author

djptek commented Apr 6, 2021

Thanks for the comments @ebeahan I've updated as discussed during the call today + applied the changes requested here in Identified potential concerns and implementation challenges/complexity

ebeahan
ebeahan previously approved these changes Apr 6, 2021
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

Other than the one nit item, LGTM 👍 !

I'll let you handle merging, @djptek. Before you do, please add a commit setting the Date at the top of the proposal doc to the date you're merging on. When you push the commit, you'll dismiss the approval ✅ , but feel free to merge without additional approval.

Typically once an RFC has advanced as stage one, another PR is opened to add new fields or changes to the experimental schema. Since the only proposed change here is updating the log.original field's description, I don't think we worry about adding the experimental schema changes.

I'm also open to discussing more if we think there might be value to including those changes, though.

rfcs/text/0017-remove-log-original.md Outdated Show resolved Hide resolved
@djptek djptek merged commit 0e1b453 into elastic:master Apr 7, 2021
@djptek djptek deleted the rfc_0017_stage_1 branch April 7, 2021 08:10
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.

3 participants