Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Introduce network.community_id #208

Merged
merged 5 commits into from
Dec 6, 2018
Merged

Introduce network.community_id #208

merged 5 commits into from
Dec 6, 2018

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Nov 30, 2018

I'm introducing this with a dot before the ID, rather than an underscore. The
standard is defined such as there can be new versions in the future, and when
that happens, we may want to introduce additional fields under community.,
to help make sense of this.

Let me know if that sounds reasonable.

Community ID spec

@webmat webmat self-assigned this Nov 30, 2018
@ruflin ruflin requested review from robgil and MikePaquette December 3, 2018 09:38
@ruflin
Copy link
Contributor

ruflin commented Dec 3, 2018

I would like to get a review on this from @MikePaquette or @robgil

Copy link
Contributor

@MikePaquette MikePaquette left a comment

Choose a reason for hiding this comment

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

I am +1 for adding this field.

Using . instead of _ in this case blips my "too-many-dots" radar.

  • I prefer using dots only when the object is a physical or logical entity that can be defined in ECS and there will be a significant set of fields under that object. I don't expect we'll define community
  • So far, in ECS, we have lowered that bar, and use a dot whenever two or more fields share a common word in their names.
  • This would be a new use, where we use a dot because we think we might need to add another field that shares a common word in the future.

If we have a good idea what the additional field might be, let's add it now, otherwise, I'd suggest that we use the _

@webmat
Copy link
Contributor Author

webmat commented Dec 4, 2018

As the spec mentions, the community ID starts with the community ID version and a colon (e.g. 1:) and is followed by the hash.

When version 2: is introduced, we may want to be able to normalize or add fields to help deal with the transition, no? Perhaps I'm solving a problem that won't exist, though.

@MikePaquette
Copy link
Contributor

@webmat OK, since we don't use the colon representation in ECS, maybe we should just define two fields now? Either:

  • network.community.id and network.community.version (a bit funny, since it's supposed to be the id version, not the community version)
  • network.community_id and network.community_id_version

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM as is.

I don't see much of a value in adding a .version since it's encoded into the ID value. If/when there is a version 2 I doubt you'd be able to meaningfully compare or aggregate v1 and v2 ID values even if the version were not embedded in the ID.

Copy link
Contributor

@robgil robgil left a comment

Choose a reason for hiding this comment

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

LGTM. Nice addition to packetbeat

@webmat
Copy link
Contributor Author

webmat commented Dec 5, 2018

I'm also of the opinion that we shouldn't add .version there yet. We're not sure what the changes to the spec will look like, and whether that would be useful. Let's wait until it happens. Using the dot just gives us "room to grow", when the growing needs to happen.

@MikePaquette Can we go with only network.community.id as is, for now?

@webmat
Copy link
Contributor Author

webmat commented Dec 5, 2018

Ping @dcode

@dcode
Copy link
Contributor

dcode commented Dec 5, 2018

I agree with @andrewkroh, the version is encoded in the value itself. There won't be any concern about comparing v1 vs v2 ID values. The only reason you'd need to parse that out is if you wanted to ask "What are all records with v1 community IDs?"

I also agree with @MikePaquette with the "too many dots radar". I'm trying to imagine other useful contexts of network.community, but failing to come up with something. That said, I could live with it.

@webmat
Copy link
Contributor Author

webmat commented Dec 5, 2018

Yeah I'm ok with going to community_id. It may indeed be a problem that won't manifest.

And to compare 1: vs 2:, people can always do a prefix search on the hash and compare both anyways.

I'll update the PR

@webmat webmat changed the title Introduce network.community.id Introduce network.community_id Dec 5, 2018
@webmat webmat merged commit 38685e0 into elastic:master Dec 6, 2018
@webmat webmat deleted the comm_id branch December 6, 2018 15:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants