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 registry fieldset #673

Merged
merged 8 commits into from
Dec 12, 2019
Merged

Add registry fieldset #673

merged 8 commits into from
Dec 12, 2019

Conversation

rw-access
Copy link
Contributor

Added field set for Windows registry.

Closes #671

@rw-access rw-access requested a review from webmat December 4, 2019 22:25
@rw-access rw-access mentioned this pull request Dec 5, 2019
40 tasks
@rw-access rw-access self-assigned this Dec 6, 2019
Copy link
Contributor

@webmat webmat 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 opening this, Ross. A few adjustments are needed, but this looks good :-)

  • We had discussed having registry.data.original, which should always be filled, and contain the base 64 encoding of the data. Did you forget to include it, or was there a reason you're not adding it yet? (either is fine)

See also the review comments for a few more small things

CHANGELOG.next.md Outdated Show resolved Hide resolved
schemas/registry.yml Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Dec 9, 2019

One more thing. The sorting on the field reference page is by short name, not by title. Which means "Windows Registry" is in the middle of the pack, instead of at the end 😂

For now simplest would be to rename title to "Registry", and omit Windows, IMO. Or if people feel like the word "Windows" is very important, perhaps we can do "Registry (Windows)".

I prefer that over changing the sorting to be based on "title", as the underlying field set will actually be "registry.*". I feel it's important that the titles follow this closely.

@rw-access
Copy link
Contributor Author

Yes, I'll add back registry.data.original, but mark it as extended. I think it's fair to make it optional, at the very least to keep storage low. The change to Registry (Windows) is probably the most clear, so I'll give that a try.

@webmat
Copy link
Contributor

webmat commented Dec 9, 2019

The way I see these potentially big fields is that users always have the power to remove them when & how they see fit. In doing that, they obviously lose the ability to utilize them later on. Elastic can even decide to make this optional & off by default in the solutions.

But in defining the schema itself, it's still useful to define the fields regardless of disk usage. Just like we did for http request/response body.

Perhaps we should add general documentation around this at some point.

@webmat
Copy link
Contributor

webmat commented Dec 11, 2019

@rw-access Do you think you'll have time to add registry.data.original soon? I'd like the descriptions for the data fields to be centered around it. E.g. 1) always store the base64 in original 2) copy to .integer when appropriate, copy to .strings when appropriate.

If you don't think you'll have time, we can still proceed and add .original later.

But we still need to fix the following before merging:

  • the field set's short: None
  • change the title to "Registry (Windows)" or equivalent, to avoid sorting surprises

Copy link
Contributor

@webmat webmat 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 adjustments.

One last discussion point, but we can merge if you're not convinced of the point below

schemas/registry.yml Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Dec 12, 2019

As discussed out of band, we're removing registry.data.integer for now, since long in ES is signed, and in the registry is unsigned.

We'll move forward and merge the rest of this PR as is, and continue thinking on what we do with the .integer field. The keyword datatype would let us query for exact values and aggregate, but wouldn't support numeric operations or sorting. So more thinking is required.

@webmat webmat merged commit 6119257 into elastic:master Dec 12, 2019
dcode pushed a commit to dcode/ecs that referenced this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows registry fieldset
2 participants