DAOS-16824 cart: lower error messages to warn level #15529

jxiong · 2024-11-21T21:37:48Z

Lower some chatty messages from error level to warn level.

Ideally the error messages should be printed by callers because they should know how serious the error is.

This PR also removes some error messages because they are being printed repeatedly due to the same error.

Signed-off-by: Jinshan Xiong [email protected]

Allow-unstable-test: true
Change-Id: I71966a235c068abbd3c74c422458a68e3a86154a

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Change-Id: I71966a235c068abbd3c74c422458a68e3a86154a

github-actions · 2024-11-21T21:38:07Z

Ticket title is 'Remove chatty error messages in cloud environment '
Status is 'Open'
Labels: 'GCP'
https://daosio.atlassian.net/browse/DAOS-16824

frostedcmos

comments inline

src/cart/crt_context.c

frostedcmos · 2024-11-21T23:31:01Z

src/cart/crt_hg.c

+		RPC_WARN(rpc_priv,
+			 "failed to invoke RPC handler, rc: "DF_RC"\n",
+			 DP_RC(rc));


i expect either crp_rpc_common_hdlr() or crt_corpc_common_hdlr() to print reason for failure if they return rc != 0, so probably we can just nuke message here altogether

Yes, I kept the error message over there. How about we change this to INFO just in case it will miss thing?

frostedcmos · 2024-11-21T23:34:30Z

src/cart/crt_hg.c

-			RPC_CERROR(crt_quiet_error(crt_cbinfo.cci_rc), DB_NET, rpc_priv,
-				   "RPC failed; rc: " DF_RC "\n", DP_RC(crt_cbinfo.cci_rc));
+			RPC_WARN(rpc_priv,
+				 "RPC failed; rc: " DF_RC "\n", DP_RC(crt_cbinfo.cci_rc));


we need something like RPC_CWARN here that takes the same condition. you dont want to print warnings for all group version mismatches either, and those are very frequent

frostedcmos · 2024-11-21T23:34:57Z

src/cart/crt_hg_proc.c

+			RPC_WARN(rpc_priv,
+				 "RPC failed to execute on target. "
+				 "error code: "DF_RC"\n", DP_RC(rc2));


same as above, you dont really want to print these as warns for DER_GRPVER either

Change-Id: I94190d2b4617a50cfbc8fad4f1a16f6973bedc1b

frostedcmos

looks good. 1 more place to change, noted inline

frostedcmos · 2024-11-22T05:39:21Z

src/cart/crt_context.c

@@ -534,8 +534,8 @@ crt_rpc_complete_and_unlock(struct crt_rpc_priv *rpc_priv, int rc)
 			cbinfo.cci_rc = rpc_priv->crp_reply_hdr.cch_rc;

 		if (cbinfo.cci_rc != 0)
-			RPC_CERROR(crt_quiet_error(cbinfo.cci_rc), DB_NET, rpc_priv,
-				   "failed, " DF_RC "\n", DP_RC(cbinfo.cci_rc));
+			RPC_WARN(rpc_priv,


this needs to be RPC_CWARN as well

frostedcmos

looks good, thanks:)

jxiong · 2024-11-22T21:13:42Z

NLT is broken - I have seen this from multiple of my PRs.

mchaarawi · 2024-11-22T21:23:14Z

i don't have a strong opinion one way or the other for this change.
we do have a lot of chatty error logging in several areas, especially timeouts.
lowering to info does not really do much, because 99% of users actually do use info logging, not ERR or WARN.

my main concern on this is what if there are actual errors on the fabric (not just timeouts). are these also downgraded to warn / info messages?

jxiong · 2024-11-22T22:22:13Z

i don't have a strong opinion one way or the other for this change. we do have a lot of chatty error logging in several areas, especially timeouts. lowering to info does not really do much, because 99% of users actually do use info logging, not ERR or WARN.

Well, at least the users would have a choice to adjust the log level now. Right now, the rate error message is at ~400 messages per second at error level.

On the server side, one of the major problem is that the same error now is printed thrice, in crt_rpc_handler_common(), crt_rpc_common_hdlr(), and crt_proc_out_common() respectively. This PR lowers the message level in crt_rpc_handler_common() and crt_proc_out_common() but keeps the error message on crt_rpc_common_hdlr().

my main concern on this is what if there are actual errors on the fabric (not just timeouts). are these also downgraded to warn / info messages?

This PR is not intended to change any internal errors in fabric. I tend to think the framework should only mind its own internal errors. The users will know how serious an error is then decide to print out something. As my example shows in the ticket, evetually the error will be printed by CLI, why would the framework bother printing those errors?

Another issue is the same as I mentioned previously - the same error is being printed over and over again in the stack. This PR does its best to remove those duplicated messages.

jolivier23 · 2024-11-25T19:28:42Z

i don't have a strong opinion one way or the other for this change. we do have a lot of chatty error logging in several areas, especially timeouts. lowering to info does not really do much, because 99% of users actually do use info logging, not ERR or WARN.

my main concern on this is what if there are actual errors on the fabric (not just timeouts). are these also downgraded to warn / info messages?

@mchaarawi our logs explorer allows us to filter messages based on the log level during or after the logs have been produced. Also, ERROR messages are marked "red" in the timeline and it would be useful to only see red when it's an actual error to make it easier to spot the root cause of real problems without having to sift through noise. Additionally, there is a psychological issue for customer who sees a bunch of errors vs warnings or informational messages. If we flag everything as an error, especially when it's not fatal, it gives a worse impression, IMO

Change-Id: I05409a9ab6187566de9e201bdaeac5ea02079136

jolivier23 · 2024-12-02T18:14:29Z

Looks like it hit some NLT issues. Try merging latest and running with

Allow-unstable-test: true in final commit message to make sure it runs full tests regardless.

mchaarawi · 2024-12-03T00:51:32Z

src/tests/simple_dfs.c

@@ -48,8 +48,16 @@ main(int argc, char **argv)
 	rc = dfs_init();


changes in this file are unrelated to the PR. a wrong merge perhaps?

daosbuild1 · 2024-12-03T01:03:03Z

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15529/7/execution/node/334/log

lower error messages to warn level

3bbc7cb

Change-Id: I71966a235c068abbd3c74c422458a68e3a86154a

jxiong requested review from a team as code owners November 21, 2024 21:37

frostedcmos reviewed Nov 21, 2024

View reviewed changes

fix per review

c8e2ac1

Change-Id: I94190d2b4617a50cfbc8fad4f1a16f6973bedc1b

frostedcmos reviewed Nov 22, 2024

View reviewed changes

missed one

cdfa636

frostedcmos previously approved these changes Nov 22, 2024

View reviewed changes

frostedcmos requested review from soumagne, mchaarawi, jgmoore-or and jolivier23 November 22, 2024 20:05

jolivier23 previously approved these changes Nov 22, 2024

View reviewed changes

jxiong requested a review from a team November 22, 2024 23:28

soumagne previously approved these changes Nov 25, 2024

View reviewed changes

jxiong added 2 commits November 26, 2024 21:05

increase the bufsize for simple_dfs

e73c6ac

Change-Id: I05409a9ab6187566de9e201bdaeac5ea02079136

fix format issue

b965d36

jxiong dismissed stale reviews from soumagne, jolivier23, and frostedcmos via b965d36 November 26, 2024 21:12

jolivier23 previously approved these changes Nov 27, 2024

View reviewed changes

somehow spaces are preferred in variable

21628a9

jxiong dismissed jolivier23’s stale review via 21628a9 November 27, 2024 18:04

frostedcmos previously approved these changes Nov 27, 2024

View reviewed changes

jxiong requested a review from soumagne November 27, 2024 23:22

jxiong requested a review from jolivier23 November 27, 2024 23:22

Merge branch 'master' into jxiong/lower_error_msgs

82a2bcc

jxiong dismissed frostedcmos’s stale review via 82a2bcc December 2, 2024 18:55

mchaarawi reviewed Dec 3, 2024

View reviewed changes

restore simple_dfs.c

ea28d57

jxiong force-pushed the jxiong/lower_error_msgs branch from 3546683 to ea28d57 Compare December 3, 2024 01:02

frostedcmos approved these changes Dec 3, 2024

View reviewed changes

jolivier23 approved these changes Dec 3, 2024

View reviewed changes

jolivier23 merged commit 9a4bec3 into master Dec 3, 2024
53 of 55 checks passed

jolivier23 deleted the jxiong/lower_error_msgs branch December 3, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16824 cart: lower error messages to warn level #15529

DAOS-16824 cart: lower error messages to warn level #15529

jxiong commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

frostedcmos left a comment

frostedcmos Nov 21, 2024

jxiong Nov 22, 2024

frostedcmos Nov 21, 2024

jxiong Nov 22, 2024

frostedcmos Nov 21, 2024

frostedcmos left a comment

frostedcmos Nov 22, 2024

frostedcmos left a comment

jxiong commented Nov 22, 2024

mchaarawi commented Nov 22, 2024

jxiong commented Nov 22, 2024 •

edited

Loading

jolivier23 commented Nov 25, 2024

jolivier23 commented Dec 2, 2024

mchaarawi Dec 3, 2024

jxiong Dec 3, 2024

daosbuild1 commented Dec 3, 2024

		@@ -48,8 +48,16 @@ main(int argc, char **argv)
		rc = dfs_init();

DAOS-16824 cart: lower error messages to warn level #15529

DAOS-16824 cart: lower error messages to warn level #15529

Conversation

jxiong commented Nov 21, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Nov 21, 2024 • edited Loading

frostedcmos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostedcmos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostedcmos left a comment

Choose a reason for hiding this comment

jxiong commented Nov 22, 2024

mchaarawi commented Nov 22, 2024

jxiong commented Nov 22, 2024 • edited Loading

jolivier23 commented Nov 25, 2024

jolivier23 commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Dec 3, 2024

jxiong commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

jxiong commented Nov 22, 2024 •

edited

Loading