Add more information in log to help troubleshooting. #1847

xluo-aws · 2024-01-09T12:44:00Z

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: xluo-aws <[email protected]>

xluo-aws · 2024-01-09T12:46:20Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

+            log.info("Fetching tool for type: " + toolType);
+
+            if (toolFactories.get(toolType) == null) {
+                log.error(toolType + " can't be found, please use ListTool API to check if it's deployed.");


Need suggestions how we should exit gracefully if the tool suggested by LLM is not available.

@arjunkumargiri do you have any suggestion here?

@arjunkumargiri, please take a look at here, it's important one.

In addition to logging, agent should respond back to LLM that the suggested tool is not available and to either use a different tool from the available list of tool or to generate response from the available data.

@jngz-es please let me know if you have any concerns.

dhrubo-os · 2024-01-09T19:12:25Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java

@@ -149,7 +149,7 @@ public static String extractModelResponseJson(String text) {
        if (matcher.find()) {
            return matcher.group(1);
        } else {
-            throw new IllegalArgumentException("Model output is invalid");
+            throw new IllegalArgumentException("Model output is invalid:" + text);


What do you think about: throw new IllegalArgumentException("Invalid model output: '" + text + "' does not meet the required criteria.");

dhrubo-os · 2024-01-09T19:13:50Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

@@ -145,7 +145,7 @@ public void run(MLAgent mlAgent, Map<String, String> params, ActionListener<Obje

                runAgent(mlAgent, params, listener, memory, memory.getConversationId());
            }, e -> {
-                log.error("Failed to get chat history", e);
+                log.error("Failed to runAgent " + mlAgent.getName(), e);


dhrubo-os · 2024-01-09T19:15:36Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

+            log.info("Fetching tool for type: " + toolType);
+
+            if (toolFactories.get(toolType) == null) {
+                log.error(toolType + " can't be found, please use ListTool API to check if it's deployed.");


@arjunkumargiri do you have any suggestion here?

Signed-off-by: xluo-aws <[email protected]> Signed-off-by: Xuesong Luo <[email protected]>

…in-myfork

Signed-off-by: Xuesong Luo <[email protected]>

sam-herman

Use of {} placeholder pattern is the standard in Java to avoid unnecessary String concatenation.

sam-herman · 2024-01-12T18:55:29Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

@@ -145,7 +145,7 @@ public void run(MLAgent mlAgent, Map<String, String> params, ActionListener<Obje

                runAgent(mlAgent, params, listener, memory, memory.getConversationId());
            }, e -> {
-                log.error("Failed to get chat history", e);
+                log.error("Failed to run agent " + mlAgent.getName(), e);


Proper use of log facility needs to use placeholders pattern instead of concatenation or manual interpolation: log.error("Failed to run agent: {} ", mlAgent.getName(), e)

sam-herman · 2024-01-12T18:56:44Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

-            log.info("Fetching tool for type: " + toolSpec.getType());
-            Tool tool = toolFactories.get(toolSpec.getType()).create(executeParams);
+            String toolType = toolSpec.getType();
+            log.info("Fetching tool for type: " + toolType);


Same thing here. Please use pattern instead of manual concatenation of text in log message:
log.info("Fetching tool for type: {}", toolType);

sam-herman · 2024-01-12T18:57:41Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLChatAgentRunner.java

+            log.info("Fetching tool for type: " + toolType);
+
+            if (toolFactories.get(toolType) == null) {
+                log.error(toolType + " can't be found, please use ListTool API to check if it's deployed.");


Same thing here, Please use pattern instead of manual concatenation of text in log message:
log.error("{} can't be found, please use ListTool API to check if it's deployed.", toolType);

b4sjoo · 2024-01-12T22:32:09Z

@xluo-aws Hi can you rebase with the main branch first and make the CI pass before we merge?

xluo-aws · 2024-01-16T07:03:21Z

@xluo-aws Hi can you rebase with the main branch first and make the CI pass before we merge?

Waiting for instruction on one of the exceptional case handling.

sam-herman

@xluo-aws can you please address my comments regarding proper usage of Log4J facility with place holders? I would like to see these comments addressed before this PR gets merged.

For reference in case the earlier comments weren't clear this is what I am talking about:
https://stackoverflow.com/questions/30975026/log4j-implicit-string-formatting

codecov · 2024-01-24T03:27:44Z

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.92%. Comparing base (d0895bb) to head (d766e0b).
Report is 348 commits behind head on main.

Files with missing lines	Patch %	Lines
.../ml/engine/algorithms/agent/MLChatAgentRunner.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1847      +/-   ##
============================================
- Coverage     82.94%   82.92%   -0.03%     
+ Complexity     5414     5412       -2     
============================================
  Files           521      521              
  Lines         21709    21712       +3     
  Branches       2213     2214       +1     
============================================
- Hits          18007    18005       -2     
- Misses         2807     2811       +4     
- Partials        895      896       +1

Flag	Coverage Δ
ml-commons	`82.92% <71.42%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dhrubo-os · 2024-03-01T08:04:35Z

@xluo-aws are you working on this issue?

Add more information in log to help troubleshooting.

5f18d83

Signed-off-by: xluo-aws <[email protected]>

xluo-aws requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee and HenryL27 as code owners January 9, 2024 12:44

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 12:44 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 12:44 — with GitHub Actions Failure

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 12:44 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 12:44 — with GitHub Actions Failure

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 12:44 — with GitHub Actions Error

xluo-aws commented Jan 9, 2024

View reviewed changes

dhrubo-os reviewed Jan 9, 2024

View reviewed changes

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 19:40 — with GitHub Actions Failure

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 19:40 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 9, 2024 19:40 — with GitHub Actions Failure

xluo-aws added 3 commits January 11, 2024 22:26

Add more information in log to help troubleshooting.

eadf1f2

Signed-off-by: xluo-aws <[email protected]> Signed-off-by: Xuesong Luo <[email protected]>

Merge branch 'main' of https://github.com/xluo-aws/ml-commons into ma…

1e3b478

…in-myfork

Update according to review comment

32e112e

Signed-off-by: Xuesong Luo <[email protected]>

xluo-aws had a problem deploying to ml-commons-cicd-env January 11, 2024 14:45 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 11, 2024 14:46 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 11, 2024 14:46 — with GitHub Actions Failure

xluo-aws had a problem deploying to ml-commons-cicd-env January 11, 2024 14:46 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 11, 2024 14:46 — with GitHub Actions Failure

sam-herman requested changes Jan 12, 2024

View reviewed changes

Merge branch 'opensearch-project:main' into main

08b0bd9

xluo-aws temporarily deployed to ml-commons-cicd-env January 16, 2024 03:44 — with GitHub Actions Inactive

xluo-aws had a problem deploying to ml-commons-cicd-env January 16, 2024 03:44 — with GitHub Actions Error

xluo-aws had a problem deploying to ml-commons-cicd-env January 16, 2024 03:44 — with GitHub Actions Failure

sam-herman requested changes Jan 16, 2024

View reviewed changes

Merge branch 'opensearch-project:main' into main

d766e0b

xluo-aws temporarily deployed to ml-commons-cicd-env January 24, 2024 03:02 — with GitHub Actions Inactive

xluo-aws temporarily deployed to ml-commons-cicd-env January 24, 2024 03:29 — with GitHub Actions Inactive

mingshl closed this Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more information in log to help troubleshooting. #1847

Add more information in log to help troubleshooting. #1847

xluo-aws commented Jan 9, 2024

xluo-aws Jan 9, 2024

dhrubo-os Jan 9, 2024

xluo-aws Jan 13, 2024

arjunkumargiri Jan 16, 2024

dhrubo-os Jan 9, 2024

dhrubo-os Jan 9, 2024

dhrubo-os Jan 9, 2024

sam-herman left a comment

sam-herman Jan 12, 2024

sam-herman Jan 12, 2024

sam-herman Jan 12, 2024

b4sjoo commented Jan 12, 2024

xluo-aws commented Jan 16, 2024

sam-herman left a comment •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading

dhrubo-os commented Mar 1, 2024

Add more information in log to help troubleshooting. #1847

Add more information in log to help troubleshooting. #1847

Conversation

xluo-aws commented Jan 9, 2024

Description

Issues Resolved

Check List

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

Choose a reason for hiding this comment

sam-herman 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

b4sjoo commented Jan 12, 2024

xluo-aws commented Jan 16, 2024

sam-herman left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

dhrubo-os commented Mar 1, 2024

sam-herman left a comment •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading