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

Adds network.protocol.name and network.protocol.id #79

Closed
wants to merge 2 commits into from

Conversation

robgil
Copy link
Contributor

@robgil robgil commented Aug 13, 2018

With most logs coming in, the protocol names are not resolved to their respective full name and instead rely on the IDs from IANA. This adds names and ids. The regular network.protocol would use name, but if you're not planning to use resolution, network.protocol.id would be available.

Rob Gil added 2 commits August 13, 2018 12:56
With most logs coming in, the protocol names are not resolved to their
respective full name and instead rely on the IDs from
[IANA](https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml)
This adds names and ids. The regular `network.protocol` would use name,
but if you're not planning to use resolution, `network.protocol.id`
would be available.
@robgil robgil requested a review from ruflin August 13, 2018 17:03
@@ -9,7 +9,17 @@
type: keyword
description: >
Network protocol name.
example: http
example: tcp
- name: protocol.name
Copy link
Member

Choose a reason for hiding this comment

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

It is not possible to have protocol and protocol.name as otherwise ingestion in ES will not work (object vs keyword type). I assume we could remove protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin ah that makes sense. I guess we can just stay with network.protocol if we plan to add network.application.protocol. That means we should be able to keep network.protocol.id.

Copy link
Member

Choose a reason for hiding this comment

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

network.protocol and network.protocol.id can also not coexist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think the confusion comes from ElasticSearch' multi-fields. So it's possible to have foo and foo.bar because it's actually one field (one value) indexed in different ways. By default/convention, the top level is analyzed for full text search, whereas the nested field is indexed another way, in this case keyword indexing, for aggregations and exact match searches. You could even have more than one nested field, but the common thread is that it's always exactly the same value being indexed in different ways.

On the other hand, you cannot have foo and foo.bar be two different values, because as @ruflin said, the top level needs to be of type object, to be able to nest arbitrary stuff under it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well actually, looking again at the actual file, it seems like protocol is just a dupe of protocol.name.

So the following should do it (we don't need to explicitly define the top level object):

   fields:
    - name: protocol.name
      type: keyword
      description: >
        Network protocol name.
      example: TCP
    - name: protocol.id
      type: keyword
      description: >
        Network protocol id https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
      example: 6

@webmat webmat mentioned this pull request Sep 18, 2018
26 tasks
@webmat
Copy link
Contributor

webmat commented Nov 5, 2018

Closing this issue, as #81 fixes this use case, and more.

@webmat webmat closed this Nov 5, 2018
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.

3 participants