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 network.name to track network names with human language :-) #30

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Jun 28, 2018

Closes #25

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file based on the

### Added
* Add `event.action` field. #21
* Add `network.name`, to track network names right in the monitoring pipeline. #25
Copy link
Member

Choose a reason for hiding this comment

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

I suggest striking right from the sentence.

@@ -5,6 +5,12 @@
description: >
Fields related to network data.
fields:
- name: name
type: text
Copy link
Member

Choose a reason for hiding this comment

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

I would have chosen keyword here because I can see myself wanting to aggregate on this term. I personally wouldn't need to have this value analyzed/searchable, but maybe someone with lots of networks would.

So perhaps we should make this a keyword and reserve the option of making it a multi-field in the future if someone wants it to be searchable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I totally agree we need to be able to aggregate on this field.

However this is absolutely a field I would want analyzed as well. E.g. Search for all "Guest" or "DMZ" networks. You don't even need such a big organization for this to make sense. As soon as you have more than one physical location you'll have multiple guest networks, as soon as you have more than one application (if you're doing things right), you'll have more than one DMZ, application, private networks. Having this field analyzed lets people filter on those in a simple manner.

I haven't looked actively, but is ECS trying to steer away from having fields indexed both ways? I'm personally not a huge fan of how things ended up, with the "field" and "field.keyword" convention. But I find I often want both types of indexing.

Copy link
Member

Choose a reason for hiding this comment

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

What we ended up with for now is having indexed by default and have the .raw convention instead. See for example: https://github.com/elastic/ecs/blob/master/schemas/url.yml#L26

More then happy to get some alternative suggestions on how to deal with it.

+1 on having this field as keyword and text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks @ruflin, I see how multi-fields is done in the schemas yml files now.

Side note: I notice that the description of field href seems to say the opposite of what the actual schema is.

Description says:

  • url.href is keyword
  • url.href.analyzed is text

Schema says:

  • url.href is text
  • url.href.raw is keyword

;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the changes from #18 were overridden by #22, which was opened before that change got merged.

Copy link
Member

Choose a reason for hiding this comment

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

@webmat @praseodym Thanks, that was not intentional.

@karenzone Could you have a look at that?

Copy link
Member

Choose a reason for hiding this comment

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

@webmat Do you plan to make it multi field in this PR or should merge as is and add multi field later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I wanted to wait for us to have this discussion next Monday. I want to discuss the naming convention for multi field and confusion I've seen it create in a past life.

@webmat
Copy link
Contributor Author

webmat commented Aug 17, 2018

One could say this was a long time coming, but this is ready for another review, and should be ready to go now ;-)

@webmat webmat added the review label Aug 17, 2018
@webmat webmat self-assigned this Aug 17, 2018
@ruflin ruflin merged commit 8e67add into elastic:master Aug 20, 2018
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.

None yet

4 participants