Added function to return pretty names on "system" module "process summary" metricset #8275

sayden · 2018-09-10T16:39:52Z

Following this discuss https://discuss.elastic.co/t/errors-in-metric-beat-log/147918/3 It's true that I found that the error message was a bit cryptic.

According to this http://thelinuxstuff.blogspot.com/2012/08/process-state-codes-in-ps-output.html the state 88, which corresponds to X state in a linux process corresponds to dead state.

I thought it could be nice to have a bit more descriptive error message in such case to help user debugging his own problems:

So, in the case of the discuss issue where the message had this format:

Unknown state <88> for process with pid 30794

It will become this:

Unknown or unexpected state <X> (dead) for process with pid 30794

BTW, I couldn't manage to compile any test, system was complaining about some redefined variables:

GOROOT=/usr/lib/go #gosetup
GOPATH=/home/caster/go #gosetup
/usr/lib/go/bin/go test -c -i -tags "darwin freebsd linux windows" -o /tmp/___TestFetch_in_github_com_elastic_beats_metricbeat_module_system_process_summary github.com/elastic/beats/metricbeat/module/system/process_summary #gosetup
# runtime
/usr/lib/go/src/runtime/lock_sema.go:30:20: active_spin redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:30:20
/usr/lib/go/src/runtime/lock_sema.go:31:20: active_spin_cnt redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:31:20
/usr/lib/go/src/runtime/lock_sema.go:32:20: passive_spin redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:32:20
/usr/lib/go/src/runtime/lock_sema.go:35:14: lock redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:46:14
/usr/lib/go/src/runtime/lock_sema.go:94:16: unlock redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:106:16
/usr/lib/go/src/runtime/lock_sema.go:124:19: noteclear redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:126:19
/usr/lib/go/src/runtime/lock_sema.go:128:20: notewakeup redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:130:20
/usr/lib/go/src/runtime/lock_sema.go:151:19: notesleep redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:139:19
/usr/lib/go/src/runtime/lock_sema.go:180:68: notetsleep_internal redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:164:45
/usr/lib/go/src/runtime/lock_sema.go:263:36: notetsleep redeclared in this block
	previous declaration at /usr/lib/go/src/runtime/lock_futex.go:210:36
/usr/lib/go/src/runtime/lock_sema.go:263:36: too many errors

jsoriano · 2018-09-10T17:59:38Z

If it is possible to have "dead" processes, as it seems by the discuss thread and this stackexchange comment, I'd propose to count or ignore them and in any case don't report them as unexpected state to don't flood logs with something that is not really an error.

jsoriano · 2018-09-10T18:02:52Z

metricbeat/module/system/process_summary/process_summary.go

@@ -96,7 +96,7 @@ func (m *MetricSet) Fetch() (common.MapStr, error) {
 		case 'Z':
 			summary.zombie++
 		default:
-			logp.Err("Unknown state <%v> for process with pid %d", state.State, pid)
+			logp.Err("Unknown or unexpected state <%c> (%s) for process with pid %d", state.State, prettyNameOf(state.State), pid)


I agree with the change to print the character value, this is much better than a random number 😄 But I'd keep this error log line only for unknown values, and not for not-so-expected states as could be the dead one.

jsoriano · 2018-09-10T18:07:59Z

Regarding the error on tests, how are you running them? this seems to be adding all platforms as tags, what will include the implementations for all platforms, this is why you see redefined declarations. You should be able to run the tests for this metricset with

go test ./metricbeat/module/system/process_summary/...

sayden · 2018-09-11T09:43:22Z

Found the solution to the test error (in the provided log you can see that it's trying to compile it to all platforms, I just needed to only set the platform linux). It was Goland that sets a lot of flags when running tests through the UI. Running them the easy way through the command line works fine.

Ok, so:

We stay with the character instead of the ASCII number.
I wouldn't hide the log. I understand that it is a hassle to flood the logs with this messages but it also helps users with debugging.
dead is, currently, an unexpected process. I wouldn't like to hide it (means, to say that it's unknown). Instead I'd prefer to count it too and report it but this causes a change in the common.MapStr schema that I don't know if it can affect somewhere (I guess it won't but I prefer to be cautious)

I still have to dig a bit more in the jenkins error, I'm not sure about what's the problem

jsoriano · 2018-09-11T10:13:51Z

I wouldn't hide the log. I understand that it is a hassle to flood the logs with this messages but it also helps users with debugging.

I meant to hide the log for the case of dead, that is a possible known case, for unknown cases we should continue logging them. My concern is that we are logging as error something that is a possible value.

dead is, currently, an unexpected process. I wouldn't like to hide it (means, to say that it's unknown). Instead I'd prefer to count it too and report it but this causes a change in the common.MapStr schema that I don't know if it can affect somewhere (I guess it won't but I prefer to be cautious)

Yes, this was one of my proposals, counting them as any other state, but in any case a process is this state is going to disappear very soon, so I find it good too to just ignore them (that is not the same as unknown, because I agree that unknown states should be logged).

Regarding changes in the fields, adding them don't break compatibility.

jsoriano · 2018-09-13T09:57:16Z

metricbeat/module/system/process_summary/process_summary.go

@@ -109,6 +112,7 @@ func (m *MetricSet) Fetch() (common.MapStr, error) {
 		"stopped":  summary.stopped,
 		"zombie":   summary.zombie,
 		"unknown":  summary.unknown,
+		"dead":     summary.dead,


You will need to add this new field to metricbeat/module/system/process_summary/_meta/fields.yml, after adding it you will also need to run make update.

Done in e4c9bab8e90c4151813763c544210eb2a08f491e

jsoriano · 2018-09-13T09:57:48Z

metricbeat/module/system/process_summary/process_summary.go

@@ -109,6 +112,7 @@ func (m *MetricSet) Fetch() (common.MapStr, error) {
 		"stopped":  summary.stopped,
 		"zombie":   summary.zombie,
 		"unknown":  summary.unknown,
+		"dead":     summary.dead,


Please, add also a changelog entry regarding this new field.

Done in e4c9bab8e90c4151813763c544210eb2a08f491e

jsoriano · 2018-09-13T15:07:24Z

CHANGELOG.asciidoc

@@ -119,6 +119,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
 - Add experimental socket summary metricset to system module {pull}6782[6782]
 - Increase ignore_above for system.process.cmdline to 2048. {pull}8101[8100]
 - Add support to renamed fields planned for redis 5.0. {pull}8167[8167]
+- Added 'died' PID state to process_system metricset on system module


Add also a reference to the pull request as in other entries.

And you will also need to update the branch with master, there is a conflict in the changelog.

Fixed both in 503c282

…cess won't be logged anymore. Change logging to show character instead of ASCII code

Added comment on fields.asciidoc

…mary" metricset (elastic#8275) Added function to return pretty names on "system" module "process summary" metricset (cherry picked from commit 457a784)

…mary" metricset (#8275) (#8312) Added function to return pretty names on "system" module "process summary" metricset

sayden added enhancement Metricbeat Metricbeat needs tests labels Sep 10, 2018

sayden requested review from jsoriano and tsg September 10, 2018 16:39

jsoriano reviewed Sep 10, 2018

View reviewed changes

jsoriano requested changes Sep 13, 2018

View reviewed changes

jsoriano reviewed Sep 13, 2018

View reviewed changes

sayden added 4 commits September 14, 2018 09:51

Added function to return pretty names on proccess states

ecb70d5

Added 'dead' to process_system outputs. Maintain logging but dead pro…

7bdf1a9

…cess won't be logged anymore. Change logging to show character instead of ASCII code

Added entry on changelog

b7186e2

Added comment on fields.asciidoc

Added forgotten reference to PR

503c282

sayden force-pushed the added-pretty-output-to-system-states branch from e4c9bab to 503c282 Compare September 14, 2018 08:01

jsoriano approved these changes Sep 14, 2018

View reviewed changes

sayden merged commit 457a784 into elastic:master Sep 14, 2018

jsoriano added needs_backport PR is waiting to be backported to other branches. and removed needs tests labels Sep 14, 2018

sayden mentioned this pull request Sep 14, 2018

Cherry-pick #8275 to 6.x: Added function to return pretty names on "system" module "process summary" metricset #8312

Merged

sayden added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Sep 14, 2018

sayden added a commit that referenced this pull request Sep 19, 2018

Added function to return pretty names on "system" module "process sum…

c5dba9d

…mary" metricset (#8275) (#8312) Added function to return pretty names on "system" module "process summary" metricset

sayden deleted the added-pretty-output-to-system-states branch September 27, 2018 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added function to return pretty names on "system" module "process summary" metricset #8275

Added function to return pretty names on "system" module "process summary" metricset #8275

sayden commented Sep 10, 2018

jsoriano commented Sep 10, 2018

jsoriano Sep 10, 2018 •

edited

Loading

jsoriano commented Sep 10, 2018

sayden commented Sep 11, 2018 •

edited

Loading

jsoriano commented Sep 11, 2018 •

edited

Loading

jsoriano Sep 13, 2018

sayden Sep 13, 2018

jsoriano Sep 13, 2018

sayden Sep 13, 2018

jsoriano Sep 13, 2018

jsoriano Sep 13, 2018

sayden Sep 14, 2018

Added function to return pretty names on "system" module "process summary" metricset #8275

Added function to return pretty names on "system" module "process summary" metricset #8275

Conversation

sayden commented Sep 10, 2018

jsoriano commented Sep 10, 2018

jsoriano Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

jsoriano commented Sep 10, 2018

sayden commented Sep 11, 2018 • edited Loading

jsoriano commented Sep 11, 2018 • edited Loading

jsoriano Sep 13, 2018

Choose a reason for hiding this comment

sayden Sep 13, 2018

Choose a reason for hiding this comment

jsoriano Sep 13, 2018

Choose a reason for hiding this comment

sayden Sep 13, 2018

Choose a reason for hiding this comment

jsoriano Sep 13, 2018

Choose a reason for hiding this comment

jsoriano Sep 13, 2018

Choose a reason for hiding this comment

sayden Sep 14, 2018

Choose a reason for hiding this comment

jsoriano Sep 10, 2018 •

edited

Loading

sayden commented Sep 11, 2018 •

edited

Loading

jsoriano commented Sep 11, 2018 •

edited

Loading