Re: [PATCH] trace-cmd: Suppress trace library warnings

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 16 Apr 2021 13:14:01 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> I can fix "-q" to suppress warnings from all trace libraries, but the
> problem is that "-q" is not set by default. These "Not being able to
> parse" messages could be very annoying in some cases and as I
> understood they are not critical ? The other problem is that not all
> trace-cmd commands have the "-q" option, but almost all of them need a
> tep handler that could cause printing of these messages.
> Yodan found one more issue, the pr_stat() libtraceevent function is
> not affected by tep_vwarning() redefine. It prints statistics on
> KernelShark startup. I think we should add some kind of priorities of
> all those library messages and decide which of them when will be
> printed.  It makes sense only for fatal library messages to be printed
> by default, if the "-q" option is not set. All others should be
> visible only if trace-cmd runs in debug mode.

In principle I agree, but I have mixed feelings about this, because these
messages have helped me in the past to fix something. And not that I was
debugging it. I would trace an event and this would warn on it, and then I
would know the event had issues.

But I do find that they are more annoying when it's showing too much,
especially on "-e all" runs. Personally, I would love to only show those
warnings if the record was on a subset of events, and not all of them.
Because when I specify events, and it fails to parse, then I really do care
about that.

I wonder if we should do the following:

On record, if its not "-e all" then add an option that tells report that
not all events are traced. And it will allow for showing of failed to parse
events by default, and quieted by "-q". If that option does not exist, then
only show the failed parsings if "--debug" is set.

That is, if the user specified events, they should care if they parse or
not. But if they just said "-e all" then they probably do not care, because
there's going to be events that do not parse.

Thoughts?


As for pr_stat(), I think we should rename it to tep_info() and tep_vinfo()
that acts just like tep_warning(), except it is for informational output
(stdout instead of stderr). This is similar to what the kernel has.

Since tep_vwarning() takes a name, so can tep_vinfo(), and I was thinking
that we should expose this string to the application.

extern const char *tep_name;

As well for libtracefs:

extern const char *tracefs_name;

Then in tep_warning(), we could do things like:

	if (name == tep_name) {
		/* do something for libtraceevent errors */
	} else if (name == tracefs_name) {
		/* do something for libtracefs errors */
	}

Instead of doing the error prone:

	if (!strcmp(name, "libtraceevent")) {
		[..]
	} else if (!strcmp(name, "libtracefs")) {
		[..]
	}

-- Steve