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

Format port numbers and numeric IDs as strings #454

Merged
merged 4 commits into from
May 22, 2019

Conversation

cwurm
Copy link

@cwurm cwurm commented May 9, 2019

It has been bugging me for some time that in Kibana things like port numbers and PIDs are displayed using a thousand separator, e.g. process.pid: 20,123.

This PR changes the display format to string where appropriate (I went through all numeric fields).

Changed fields are:

  1. client.port
  2. destination.port
  3. event.severity
  4. event.sequence
  5. http.response.status_code
  6. process.pid
  7. process.ppid
  8. process.pgid
  9. process.thread.id
  10. server.port
  11. source.port
  12. url.port

@cwurm cwurm added the review label May 9, 2019
@cwurm cwurm requested a review from webmat May 9, 2019 19:16
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Nice, didn't even know we have this option. Could you add a changelog entry.

LGTM

@webmat
Copy link
Contributor

webmat commented May 10, 2019

This has been bugging me for a long time as well! Thanks for submitting this.

I'll try to review and merge next week :-)

@cwurm
Copy link
Author

cwurm commented May 20, 2019

Could you add a changelog entry.

Done.

@webmat @ruflin Let me know if this looks good.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. I would say lets get it merged.

I also resolved the merge conflict (hopefully).

@webmat
Copy link
Contributor

webmat commented May 22, 2019

I'm all for rendering more straightforward numbers for things that should be easy to copy / paste around, such as PIDs, and the other ones you're improving here. This was much needed 👍

But I have one worry: does formatting numbers as a string have an impact on what we can do with the field in visualizations? Because we can also use a numeral.js format for number fields, such as '0[.]000' to render integers with no noise, and optional decimals. See below.

I'm not a huge fan of having Kibana implementation details in ECS, for obvious reasons 😆 But #425 is how I ended up mitigating an issue with event.duration, during the Beats migration. So it's totally possible to dictate a bit more of the format all the way from here.

The fact that it's a really Kibana-specific implementation detail I think is acceptable for now. If it becomes a problem, we can always add a layer of indirection in the future 😉

noiseless-numerals

@cwurm
Copy link
Author

cwurm commented May 22, 2019

But I have one worry: does formatting numbers as a string have an impact on what we can do with the field in visualizations? Because we can also use a numeral.js format for number fields, such as '0[.]000' to render integers with no noise, and optional decimals. See below.

Formats do not affect how fields can be queried or aggregated on. It's still possible to create a histogram on any of these fields, or to filter for low port numbers in Discover (e.g. destination.port < 1000).

@cwurm cwurm merged commit 0a43031 into elastic:master May 22, 2019
@cwurm cwurm deleted the format_as_string branch May 23, 2019 17:35
@webmat webmat assigned webmat and unassigned webmat May 23, 2019
@webmat webmat added the v1.0.2 label May 23, 2019
webmat pushed a commit to webmat/ecs that referenced this pull request May 23, 2019
Changes the display format of things like port numbers and PIDs to string where appropriate. Changed fields are:

client.port
destination.port
event.severity
event.sequence (cherry-pick note: not in 1.0)
http.response.status_code
process.pid
process.ppid
process.pgid (cherry-pick note: not in 1.0)
process.thread.id
server.port
source.port
url.port
webmat pushed a commit to webmat/ecs that referenced this pull request May 24, 2019
Changes the display format of things like port numbers and PIDs to string where appropriate. Changed fields are:

client.port
destination.port
event.severity
event.sequence (cherry-pick note: not in 1.0)
http.response.status_code
process.pid
process.ppid
process.pgid (cherry-pick note: not in 1.0)
process.thread.id
server.port
source.port
url.port
webmat pushed a commit to webmat/ecs that referenced this pull request May 24, 2019
Changes the display format of things like port numbers and PIDs to string where appropriate. Changed fields are:

client.port
destination.port
event.severity
event.sequence (cherry-pick note: not in 1.0)
http.response.status_code
process.pid
process.ppid
process.pgid (cherry-pick note: not in 1.0)
process.thread.id
server.port
source.port
url.port
webmat pushed a commit that referenced this pull request May 24, 2019
…454)

Backport of PR #467 to 1.0 branch. Original message:

Changes the display format of things like port numbers and PIDs to string where appropriate. Changed fields are:

client.port
destination.port
event.severity
event.sequence (cherry-pick note: not in 1.0)
http.response.status_code
process.pid
process.ppid
process.pgid (cherry-pick note: not in 1.0)
process.thread.id
server.port
source.port
url.port
webmat pushed a commit to webmat/ecs that referenced this pull request May 24, 2019
…trings (elastic#467)

Backport of PR elastic#454 to 1.0 branch. Original message:

Changes the display format of things like port numbers and PIDs to string where appropriate. Changed fields are:

client.port
destination.port
event.severity
event.sequence (cherry-pick note: not in 1.0)
http.response.status_code
process.pid
process.ppid
process.pgid (cherry-pick note: not in 1.0)
process.thread.id
server.port
source.port
url.port
@webmat webmat added v1.0.1 and removed v1.0.2 labels May 24, 2019
webmat pushed a commit to webmat/ecs that referenced this pull request May 24, 2019
webmat pushed a commit to webmat/ecs that referenced this pull request May 24, 2019
@Randy-312
Copy link

Have you talked to the SIEM folks?
Being able to do some of the aggregations and queries on port numbers seems extremely useful in that realm. I'd hate to see you limit what can be done with ECS in the Security realm.

@cwurm
Copy link
Author

cwurm commented Aug 5, 2019

@Randy-312 I'm on the SIEM team :) Changing the display format does not affect the type of queries or aggregations you can run on these fields.

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.

4 participants