send execution error field in execution log API#4332
Merged
prashelke merged 12 commits intoOct 16, 2025
Conversation
Contributor
WalkthroughUpdated AccountReport contract package; refactored centralized account-report API handler to accept ExecutionLogRequest and added GET helpers; HttpLogAppender now batches and flushes on dispose, sending consolidated ExecutionLogRequest(s); ExecutionProgressReporterListener accepts optional sourcePath; minor POM whitespace edit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Log4Net as HttpLogAppender
participant API as AccountReportApiHandler
participant Central as Central DB API
App->>Log4Net: Emit log event(s)
Note over Log4Net: Detect runset phases\nAccumulate currentLog & ExecutionErrorRequests
Log4Net->>Log4Net: Batch on threshold/timeout/shutdown -> Build ExecutionLogRequest
alt ExecutionId present
Log4Net->>API: SendExecutionLogToCentralDBAsync(ExecutionLogRequest)
API->>Central: POST ExecutionLogRequest
Central-->>API: Ack / Error
API-->>Log4Net: Success / Failure
end
sequenceDiagram
autonumber
participant Utils as GingerRemoteExecutionUtils
participant Endpoint as GingerPlayEndPointManager
participant API as AccountReportApiHandler
participant Central as Central DB API
Utils->>Endpoint: GetAccountReportServiceUrl()
Utils->>API: GetRunsetExecutionDataByRunSetIDFromCentralDB(solutionId, runSetId)
API->>Central: GET runset execution data
Central-->>API: HTML/JSON string
API-->>Utils: response string
Utils->>Utils: ConvertResponsInRunsetReport(response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-05T10:50:57.706ZApplied to files:
🧬 Code graph analysis (1)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
661-688: Harden the REST call (timeouts, headers, diagnostics)Add explicit timeouts and Accept header to avoid indefinite hangs and improve observability. Consider logging StatusCode and ResponseUri on failure.
Example (non-breaking suggestion):
- Set RestClientOptions.MaxTimeout in the constructor.
- Add Accept: application/json header to the request.
- Include response.StatusCode in error logs.
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
71-73: Initialize handler after computing the final ApiUrl (order bug)Handler is constructed before appending the gateway, so its RestClient uses the wrong base URL.
Apply this diff:
- _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway + _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; // Final URL with gateway + _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl);
262-359: Fix state handling, null-cast, and remove unused builder; prevent ErrorSource overwriteIssues:
- State flags (pre/post) are never reset; can misclassify later errors.
- ExecutionId is cast to Guid before null-check, can throw.
- errlogDataBuilder is unused.
- Pre/Post ErrorSource is overwritten by SetExecutionError.
Apply this diff:
- var errlogDataBuilder = new StringBuilder(); List<AccountReport.Contracts.RequestModels.ExecutionErrorRequest> ExecutionErrorRequestsList = new List<ExecutionErrorRequest>(); @@ - if (Regex.IsMatch(evt.RenderedMessage, @"Running Pre-Execution Run Set Operations")) + if (Regex.IsMatch(evt.RenderedMessage, @"Running Pre-Execution Run Set Operations")) { - isPreRunSetOperation = true; + isPreRunSetOperation = true; + isPostRunSetOperation = false; } @@ - if (Regex.IsMatch(evt.RenderedMessage, @"Running Post-Execution Run Set Operations")) + if (Regex.IsMatch(evt.RenderedMessage, @"Running Post-Execution Run Set Operations")) { - isPostRunSetOperation = true; + isPostRunSetOperation = true; + isPreRunSetOperation = false; } @@ - if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Ended")) + if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Ended")) { isExecutionStarted = false; + isPreRunSetOperation = false; + isPostRunSetOperation = false; + } + // Reset pre/post flags on operation end + if (Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation")) + { + isPreRunSetOperation = false; + isPostRunSetOperation = false; } @@ - if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && Regex.IsMatch(evt.RenderedMessage, @"Error\(s\) occurred process exit code")) + if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && Regex.IsMatch(evt.RenderedMessage, @"Error\(s\) occurred process exit code")) { - AccountReportRunSet accountReportRunSet = new AccountReportRunSet - { - Id = (Guid)ExecutionId, - ExecutionId = (Guid)ExecutionId, - EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, - GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, - Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, - RunStatus = eExecutionStatus.Failed, - }; - if(ExecutionId != null) + if (ExecutionId != null) { + AccountReportRunSet accountReportRunSet = new AccountReportRunSet + { + Id = (Guid)ExecutionId, + ExecutionId = (Guid)ExecutionId, + EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, + GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, + Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, + RunStatus = eExecutionStatus.Failed, + }; bool Response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, true); if(!Response) { Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Failed to send Runset Execution data to Central DB."); } } @@ - else if (isPreRunSetOperation) + else if (isPreRunSetOperation) { - if(Regex.IsMatch(evt.Level.DisplayName,@"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed")) + if(Regex.IsMatch(evt.Level.DisplayName,@"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed")) { - ExecutionErrorRequests.ErrorSource = "Operation type Pre"; - SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + ExecutionErrorRequests.ErrorSource = "Operation type Pre"; } } @@ - else if (isPostRunSetOperation) + else if (isPostRunSetOperation) { if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed")) { - ExecutionErrorRequests.ErrorSource = "Operation type Post"; - SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + ExecutionErrorRequests.ErrorSource = "Operation type Post"; } } @@ - string ErrorLogData = errlogDataBuilder.ToString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Ginger/Ginger/DLLs/AccountReport.Contracts.dllis excluded by!**/*.dll,!**/*.dll
📒 Files selected for processing (7)
Ginger/Ginger/Ginger.csproj(1 hunks)Ginger/GingerCoreNET/GingerCoreNET.csproj(1 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(2 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs(2 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(6 hunks)Ginger/GingerCoreNETUnitTest/GingerCoreNETUnitTest.csproj(1 hunks)Ginger/GingerTest/GingerTest.csproj(1 hunks)
🔇 Additional comments (8)
Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs (1)
276-276: No-op formatting change.Ginger/GingerTest/GingerTest.csproj (1)
48-52: Same here: protect the DLL reference and copy to output.Wrap with Exists and set Private to ensure CopyLocal.
- <ItemGroup> - <Reference Include="AccountReport.Contracts"> - <HintPath>..\Ginger\DLLs\AccountReport.Contracts.dll</HintPath> - </Reference> - </ItemGroup> + <ItemGroup> + <Reference Include="AccountReport.Contracts" Condition="Exists('..\Ginger\DLLs\AccountReport.Contracts.dll')"> + <HintPath>..\Ginger\DLLs\AccountReport.Contracts.dll</HintPath> + <Private>true</Private> + </Reference> + </ItemGroup>Run to verify HintPath existence across projects:
Ginger/Ginger/Ginger.csproj (1)
783-785: Add Exists guard and CopyLocal.Protect against missing DLL; ensure it’s copied to output.
- <Reference Include="AccountReport.Contracts"> - <HintPath>DLLs\AccountReport.Contracts.dll</HintPath> - </Reference> + <Reference Include="AccountReport.Contracts" Condition="Exists('DLLs\AccountReport.Contracts.dll')"> + <HintPath>DLLs\AccountReport.Contracts.dll</HintPath> + <Private>true</Private> + </Reference>If you prefer a single source of truth for the DLL path, consider defining a property in Directory.Build.props (e.g., ) and referencing it here to avoid drift across projects.
Ginger/GingerCoreNETUnitTest/GingerCoreNETUnitTest.csproj (1)
130-134: Harden local DLL reference.Add Exists guard and set Private to ensure the DLL is copied to bin.
- <Reference Include="AccountReport.Contracts"> - <HintPath>..\Ginger\DLLs\AccountReport.Contracts.dll</HintPath> - </Reference> + <Reference Include="AccountReport.Contracts" Condition="Exists('..\Ginger\DLLs\AccountReport.Contracts.dll')"> + <HintPath>..\Ginger\DLLs\AccountReport.Contracts.dll</HintPath> + <Private>true</Private> + </Reference>Consider committing the DLL or setting up a local feed/CI artifact to guarantee availability for contributors and build agents.
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
34-38: Necessary imports addedNew usings for Regex and AccountReport contracts look correct for the new error-capture flow.
197-198: Correctly preserves exception context in LoggingEventPassing ExceptionObject into the LoggingEvent constructor keeps stack traces intact. LGTM.
368-375: Good integration of ExecutionErrorRequests into ExecutionLogRequestMapping ExecutionId, InstanceId, LogData, and errors into a single payload aligns with the new API. LGTM.
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
618-659: Don’t drop error‑only payloads; remove hard‑coded host override; fix LogId usage in log message
- Current guard skips sending when LogData is empty, which prevents sending ExecutionErrorRequests alone. This breaks the PR’s goal to send execution errors.
- Hardcoded Replace to localhost is an environment leak and will misroute prod/stage traffic.
- Logging executionLogRequest.LogId will not compile if the contract lacks this property.
Apply the following diff:
- if (string.IsNullOrWhiteSpace(executionLogRequest.LogData)) + // Allow send when either logs or errors exist + if (string.IsNullOrWhiteSpace(executionLogRequest.LogData) + && (executionLogRequest.ExecutionErrorRequests == null + || executionLogRequest.ExecutionErrorRequests.Count == 0)) { - Reporter.ToLog(eLogLevel.DEBUG, "SendExecutionLogToCentralDBAsync: logData is empty, skipping"); + Reporter.ToLog(eLogLevel.DEBUG, "SendExecutionLogToCentralDBAsync: no log data or execution errors to send, skipping"); return false; } @@ - string message = string.Format("execution log to Central DB (API URL:'{0}', Execution Id:'{1}', Instance Id:'{2}', Log Id:'{3}')", apiUrl, executionLogRequest.ExecutionId, executionLogRequest.InstanceId, executionLogRequest.LogId); + string message = string.Format("execution log to Central DB (API URL:'{0}', Execution Id:'{1}', Instance Id:'{2}')", apiUrl, executionLogRequest.ExecutionId, executionLogRequest.InstanceId); @@ - string FinalAPIUrl = $"{apiUrl.TrimEnd('/')}/{SEND_EXECUTIONLOG}"; - FinalAPIUrl = FinalAPIUrl.Replace("https://usstlattstl01:9001/ginger-report", "https://localhost:3117"); + string FinalAPIUrl = $"{apiUrl.TrimEnd('/')}/{SEND_EXECUTIONLOG}";
- Optional: if you need a request correlation id, generate it locally (Guid.NewGuid) rather than relying on ExecutionLogRequest.LogId unless it exists in the contract.
To confirm contract shape, please run:
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
384-405: Avoid logging from inside the appender to prevent re-entrancyReporter.ToLog inside the appender can re-enqueue log events and create feedback loops. Consider guarding with a re-entrancy flag or routing these diagnostics to a different sink.
144-176: Flush remaining buffer on Dispose/OnCloseDispose cancels the worker and waits briefly but doesn’t flush any remaining buffered events. Add a final drain-and-send to minimize log loss on shutdown.
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
423-430: Don’t overwrite ErrorSource/OriginPath; enrich from logger/locationSetExecutionError overwrites ErrorSource/OriginPath, clobbering values you set earlier (“Operation type Pre/Post”). Also, both fields equal RenderedMessage, losing origin context. Prefer LoggerName and LocationInformation, and only set when empty.
- private static void SetExecutionError(LoggingEvent evt, ExecutionErrorRequest ExecutionErrorRequests,bool isExecutionStarted) + private static void SetExecutionError(LoggingEvent evt, ExecutionErrorRequest ExecutionErrorRequests, bool isExecutionStarted) { ExecutionErrorRequests.ErrorOccurrenceTime = evt.TimeStamp; ExecutionErrorRequests.ErrorLevel = isExecutionStarted ? eExecutionErrorLevel.Execution : eExecutionErrorLevel.Setup; - ExecutionErrorRequests.ErrorSource = evt.RenderedMessage; - ExecutionErrorRequests.ErrorOriginPath = evt.RenderedMessage; + if (string.IsNullOrWhiteSpace(ExecutionErrorRequests.ErrorSource)) + { + ExecutionErrorRequests.ErrorSource = evt.LoggerName; + } + if (string.IsNullOrWhiteSpace(ExecutionErrorRequests.ErrorOriginPath)) + { + var li = evt.LocationInformation; + ExecutionErrorRequests.ErrorOriginPath = li != null ? $"{li.FileName}:{li.LineNumber}" : evt.LoggerName; + } ExecutionErrorRequests.ErrorMessage = evt.RenderedMessage; }This preserves “Operation type Pre/Post” when you set it before calling SetExecutionError and provides meaningful origin info. Based on learnings
Also applies to: 341-343, 349-351
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(6 hunks)
🔇 Additional comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
274-287: Reset pre/post flags to avoid stale state and misclassificationisPreRunSetOperation and isPostRunSetOperation are never reset, which can misclassify later errors. Reset counterpart flags when entering one, clear both on Run Set end, and clear after Run Set Operation end.
- if (Regex.IsMatch(evt.RenderedMessage, @"Running Pre-Execution Run Set Operations")) + if (Regex.IsMatch(evt.RenderedMessage, @"Running Pre-Execution Run Set Operations")) { - isPreRunSetOperation = true; + isPreRunSetOperation = true; + isPostRunSetOperation = false; } - if (Regex.IsMatch(evt.RenderedMessage, @"Running Post-Execution Run Set Operations")) + if (Regex.IsMatch(evt.RenderedMessage, @"Running Post-Execution Run Set Operations")) { - isPostRunSetOperation = true; + isPostRunSetOperation = true; + isPreRunSetOperation = false; } - if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Ended")) + if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Ended")) { - isExecutionStarted = false; + isExecutionStarted = false; + isPreRunSetOperation = false; + isPostRunSetOperation = false; }- ExecutionErrorRequests.ErrorSource = "Operation type Pre"; - SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + ExecutionErrorRequests.ErrorSource = "Operation type Pre"; + SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + isPreRunSetOperation = false;- ExecutionErrorRequests.ErrorSource = "Operation type Post"; - SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + ExecutionErrorRequests.ErrorSource = "Operation type Post"; + SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); + isPostRunSetOperation = false;Consider evaluating pre/post branches irrespective of isExecutionStarted if phases can overlap.
Also applies to: 279-282, 339-343, 347-351
68-73: Fix ApiUrl setter ordering to avoid handler using wrong URLYou create AccountReportApiHandler with the pre-gateway URL, then mutate _apiUrl to the final gateway URL. This can send RunSet data via a different base than logs. Build the final URL first, then instantiate the handler.
set { if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) { - _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway + var finalUrl = string.IsNullOrWhiteSpace(value) ? null : $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; + _apiUrl = finalUrl; // Final URL with gateway + _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); } }Also verify consistency with ApiUrl getter, which uses GetAccountReportServiceUrlByGateWay() (might already include the gateway).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (5)
263-264: Remove unused errlogDataBuilder and ErrorLogData variables.The
errlogDataBuildervariable declared at line 263 is never appended to, andErrorLogDataat line 369 is assigned but never used. These are dead code remnants.This issue was previously flagged. Apply this diff to remove the unused declarations:
if (buffer.Count >= GetBatchSize() || (buffer.Count > 0 && log == null)) { var logDataBuilder = new StringBuilder(); - var errlogDataBuilder = new StringBuilder(); List<AccountReport.Contracts.RequestModels.ExecutionErrorRequest> ExecutionErrorRequestsList = new List<ExecutionErrorRequest>(); foreach (var evt in buffer)And remove the unused assignment at line 369:
} string LogData = logDataBuilder.ToString(); - string ErrorLogData = errlogDataBuilder.ToString(); if (AccountReportApiHandler != null)Based on learnings.
270-289: Replace Regex.IsMatch with simpler string comparisons for performance.Using
Regex.IsMatchfor exact substring matches adds unnecessary overhead in the logging hot path. These patterns can be replaced withContainsorIndexOfchecks.This issue was previously flagged. Apply these changes:
- if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Started")) + if (evt.RenderedMessage.Contains("Run Set Execution Started", StringComparison.Ordinal)) { isExecutionStarted = true; } - if (Regex.IsMatch(evt.RenderedMessage, @"Running Pre-Execution Run Set Operations")) + if (evt.RenderedMessage.Contains("Running Pre-Execution Run Set Operations", StringComparison.Ordinal)) { isPreRunSetOperation = true; } - if (Regex.IsMatch(evt.RenderedMessage, @"Running Post-Execution Run Set Operations")) + if (evt.RenderedMessage.Contains("Running Post-Execution Run Set Operations", StringComparison.Ordinal)) { isPostRunSetOperation = true; isPreRunSetOperation = false; } - if (Regex.IsMatch(evt.RenderedMessage, @"Run Set Execution Ended")) + if (evt.RenderedMessage.Contains("Run Set Execution Ended", StringComparison.Ordinal)) { isExecutionStarted = false; }Based on learnings.
328-361: Optimize Regex usage for level and message matching.Similar to lines 270-289, these Regex patterns can be replaced with simpler string operations for better performance.
Apply this diff:
- if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && !isExecutionStarted) + if (evt.Level.DisplayName.Equals("ERROR", StringComparison.Ordinal) && !isExecutionStarted) { SetExecutionError(evt, ExecutionErrorRequests,isExecutionStarted); } else if (isExecutionStarted) { - if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage,@"Action Execution Ended.*Execution Status= Failed",RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Action Execution Ended", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Execution Status= Failed", StringComparison.Ordinal)) { var match = Regex.Match(evt.RenderedMessage, @"SourcePath:\s*(.+?)\)"); if (match.Success) { string sourcePath = match.Groups[1].Value; ExecutionErrorRequests.ErrorOriginPath = sourcePath; } SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); } } else if (isPreRunSetOperation) { - if(Regex.IsMatch(evt.Level.DisplayName,@"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Execution Ended for Run Set Operation", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Status= Failed", StringComparison.Ordinal)) { ExecutionErrorRequests.ErrorSource = "Operation type Pre"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); } } else if (isPostRunSetOperation) { - if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Execution Ended for Run Set Operation", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Status= Failed", StringComparison.Ordinal)) { ExecutionErrorRequests.ErrorSource = "Operation type Post"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted); } }Note: The Regex.Match call at line 336 for extracting SourcePath should remain as it requires pattern capture.
Based on learnings.
430-435: Consider enriching error origin fields.The
SetExecutionErrormethod currently only setsErrorOccurrenceTime,ErrorLevel, andErrorMessage. TheErrorOriginPathis only populated in specific cases (line 341), andErrorSourceis set for operation types (lines 350, 358) but not in other cases. Consider populating these fields more consistently.For example, you could extract method/class information from
evt.LocationInformationorevt.LoggerNameto populateErrorSourcewhen not already set:private static void SetExecutionError(LoggingEvent evt, ExecutionErrorRequest ExecutionErrorRequests, bool isExecutionStarted) { ExecutionErrorRequests.ErrorOccurrenceTime = evt.TimeStamp; ExecutionErrorRequests.ErrorLevel = isExecutionStarted ? eExecutionErrorLevel.Execution : eExecutionErrorLevel.Setup; ExecutionErrorRequests.ErrorMessage = evt.RenderedMessage; + + // Populate ErrorSource if not already set + if (string.IsNullOrEmpty(ExecutionErrorRequests.ErrorSource)) + { + ExecutionErrorRequests.ErrorSource = evt.LoggerName ?? "Unknown"; + } + + // Populate ErrorOriginPath from location info if available and not already set + if (string.IsNullOrEmpty(ExecutionErrorRequests.ErrorOriginPath) && evt.LocationInformation != null) + { + ExecutionErrorRequests.ErrorOriginPath = $"{evt.LocationInformation.ClassName}.{evt.LocationInformation.MethodName}"; + } }Based on learnings.
291-311: CRITICAL: Null-safety violation - casting nullable ExecutionId before null check.Casting
ExecutionId(which isGuid?) toGuidat lines 295-296 will throwInvalidOperationExceptionifExecutionIdis null. The null check at line 302 comes too late. This is a critical runtime error that will crash the application.This issue was marked as addressed in commit a7a2ab3 but the problematic code is still present. Apply this diff to fix:
- if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && Regex.IsMatch(evt.RenderedMessage, @"Error\(s\) occurred process exit code")) + if (evt.Level.DisplayName.Equals("ERROR", StringComparison.Ordinal) && + evt.RenderedMessage.Contains("Error(s) occurred process exit code", StringComparison.Ordinal)) { - AccountReportRunSet accountReportRunSet = new AccountReportRunSet + if (ExecutionId.HasValue) { - Id = (Guid)ExecutionId, - ExecutionId = (Guid)ExecutionId, - EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, - GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, - Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, - RunStatus = eExecutionStatus.Failed, - }; - if(ExecutionId != null) - { - bool Response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, true); - if(!Response) + var accountReportRunSet = new AccountReportRunSet + { + Id = ExecutionId.Value, + ExecutionId = ExecutionId.Value, + EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, + GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, + Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, + RunStatus = eExecutionStatus.Failed, + }; + var response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, true); + if (!response) { Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Failed to send Runset Execution data to Central DB."); } } - }Note: This also applies the Regex optimization mentioned in the previous comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(2 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(6 hunks)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
62-63: Use a fresh LogId for every payload.
LogIdis generated once per handler, so every batch goes out with the same identifier. If the API treatsLogIdas a unique key (common for execution-log inserts), later batches will overwrite or be rejected, so you silently lose data. Generate the GUID per send instead of caching it on the instance.- private readonly Guid LogId = Guid.NewGuid(); ... - executionLogRequest.LogId = LogId; + executionLogRequest.LogId = Guid.NewGuid();Also applies to: 635-636
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
288-304: GuardExecutionIdbefore casting.
ExecutionIdis nullable; the(Guid)ExecutionIdcast will throw immediately when it’s null, so we never reach the null-check and the appender blows up during normal runs without an ID. Only build the payload after confirmingExecutionId.HasValue.- if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && Regex.IsMatch(evt.RenderedMessage, @"Error\(s\) occurred process exit code")) - { - AccountReportRunSet accountReportRunSet = new AccountReportRunSet - { - Id = (Guid)ExecutionId, - ExecutionId = (Guid)ExecutionId, - EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, - GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, - Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, - RunStatus = eExecutionStatus.Failed, - }; - if (ExecutionId != null) - { + if (Regex.IsMatch(evt.Level.DisplayName, @"^ERROR$") && Regex.IsMatch(evt.RenderedMessage, @"Error\(s\) occurred process exit code")) + { + if (ExecutionId.HasValue) + { + AccountReportRunSet accountReportRunSet = new AccountReportRunSet + { + Id = ExecutionId.Value, + ExecutionId = ExecutionId.Value, + EntityId = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Guid, + GingerSolutionGuid = WorkSpace.Instance.Solution.Guid, + Name = WorkSpace.Instance.RunsetExecutor?.RunSetConfig?.Name, + RunStatus = eExecutionStatus.Failed, + }; bool Response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, true); if (!Response) { Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Failed to send Runset Execution data to Central DB."); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Ginger/Ginger/Ginger.csproj(1 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(2 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs(2 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
Task(239-422)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs (2)
41-52: Avoid gateway-only gating and remove unused HttpClient
- The if-condition requires GingerPlayUtils.IsGingerPlayGatewayUrlConfigured(), which blocks fallback URLs returned by GingerPlayEndPointManager.GetAccountReportServiceUrl(). Use only the non-empty baseURI check.
- The HttpClient instance is never used; remove the using block and dependency.
Apply:
- if (!string.IsNullOrEmpty(baseURI) && WorkSpace.Instance.Solution.LoggerConfigurations.PublishLogToCentralDB == ExecutionLoggerConfiguration.ePublishToCentralDB.Yes && GingerPlayUtils.IsGingerPlayGatewayUrlConfigured()) + if (!string.IsNullOrEmpty(baseURI) && WorkSpace.Instance.Solution.LoggerConfigurations.PublishLogToCentralDB == ExecutionLoggerConfiguration.ePublishToCentralDB.Yes) { - using (var httpClient = new HttpClient()) - { - AccountReportApiHandler accountReportApiHandler = new AccountReportApiHandler(GingerPlayEndPointManager.GetAccountReportServiceUrl()); - var response = accountReportApiHandler.GetRunsetExecutionDataBySolutionIDFromCentralDB(soluionGuid); - - if(!string.IsNullOrEmpty(response)) - { - runSetReports = ConvertResponsInRunsetReport(response); - } - } + var apiUrl = baseURI.TrimEnd('/'); + var accountReportApiHandler = new AccountReportApiHandler(apiUrl); + var response = accountReportApiHandler.GetRunsetExecutionDataBySolutionIDFromCentralDB(soluionGuid); + if (!string.IsNullOrEmpty(response)) + { + runSetReports = ConvertResponsInRunsetReport(response); + } }Repeat similar change in GetRunsetExecutionInfo. Also remove unused
using System.Net.Http;.
84-107: Harden JSON null and DateTime parsing to avoid runtime exceptions
- JsonConvert.DeserializeObject can return null; foreach over null will throw.
- DateTime.Parse can throw or convert incorrectly if Kind is unspecified.
Apply:
- var runsetHLInfoResponses = JsonConvert.DeserializeObject<List<RunsetHLInfoResponse>>(result); - List<RunSetReport> runSetReports = []; - - foreach (var runsetHLInfo in runsetHLInfoResponses) + var runsetHLInfoResponses = JsonConvert.DeserializeObject<List<RunsetHLInfoResponse>>(result) ?? new List<RunsetHLInfoResponse>(); + List<RunSetReport> runSetReports = []; + + foreach (var runsetHLInfo in runsetHLInfoResponses) { Enum.TryParse(runsetHLInfo.Status, out Execution.eRunStatus runStatus); + DateTime startUtc, endUtc; + if (!DateTime.TryParse(runsetHLInfo.StartTime, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, out startUtc)) + { + startUtc = DateTime.MinValue; + } + if (!DateTime.TryParse(runsetHLInfo.EndTime, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, out endUtc)) + { + endUtc = DateTime.MinValue; + } runSetReports.Add(new RunSetReport() { GUID = runsetHLInfo.ExecutionID.ToString(), RunSetGuid = runsetHLInfo.EntityId, Name = runsetHLInfo.RunsetName, Description = "", - StartTimeStamp = DateTime.Parse(runsetHLInfo.StartTime, CultureInfo.InvariantCulture).ToUniversalTime(), - EndTimeStamp = DateTime.Parse(runsetHLInfo.EndTime, CultureInfo.InvariantCulture).ToUniversalTime(), + StartTimeStamp = startUtc, + EndTimeStamp = endUtc, Elapsed = runsetHLInfo.Duration,Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionProgressReporterListener.cs (1)
97-170: Log SourcePath even when obj is null at End phaseCurrently SourcePath is appended only when obj != null. Move SourcePath appending outside the obj != null guard (still limited to End).
- //get the execution fields and their values - if (objExecutionPhase == eExecutionPhase.End && obj != null) + //get the execution fields and their values + if (objExecutionPhase == eExecutionPhase.End && obj != null) { stringBuilder.Append("Details:").AppendLine(); try { PropertyInfo[] props = obj.GetType().GetProperties(); foreach (PropertyInfo prop in props) { try { FieldParamsFieldType attr = ((FieldParamsFieldType)prop.GetCustomAttribute(typeof(FieldParamsFieldType))); if (attr == null) { continue; } FieldsType ftype = attr.FieldType; if (ftype == FieldsType.Field) { string propName = prop.Name; string propFullName = ((FieldParamsNameCaption)prop.GetCustomAttribute(typeof(FieldParamsNameCaption))).NameCaption; object propValueobj = prop.GetValue(obj);//obj.GetType().GetProperty(propName, BindingFlags.Public | BindingFlags.Instance).GetValue(obj).ToString(); string propValueStr = ""; if (propValueobj != null) { //special case for execution time convertion from UTC format if (Attribute.IsDefined(prop, typeof(UsingUTCTimeFormat))) { try { propValueStr = ((DateTime)propValueobj).ToLocalTime().ToString("dd-MMM-yy HH:mm:ss"); } catch { propValueStr = propValueobj.ToString(); } } else { propValueStr = propValueobj.ToString(); } } stringBuilder.Append(propFullName).Append("= ").Append(propValueStr).AppendLine(); } } catch (Exception ex) { } } - - if(!string.IsNullOrEmpty(sourcePath)) - { - stringBuilder.Append("SourcePath").Append("= ").Append(sourcePath).AppendLine(); - } } catch (Exception) { } } + if (objExecutionPhase == eExecutionPhase.End && !string.IsNullOrEmpty(sourcePath)) + { + stringBuilder.Append("SourcePath").Append("= ").Append(sourcePath).AppendLine(); + }Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
54-71: Fix ApiUrl initialization/normalization and AccountReportApiHandler base URL
- Getter should fall back to GetAccountReportServiceUrl(), not gateway-only.
- Setter builds AccountReportApiHandler with the normalized URL; current order creates handler with non-final value.
- Avoid blindly appending the gateway segment if value already includes it.
Apply:
get { - if (string.IsNullOrEmpty(_apiUrl)) - { - if (!string.IsNullOrWhiteSpace(GingerPlayEndPointManager.GetAccountReportServiceUrlByGateWay())) - { - _apiUrl = GingerPlayEndPointManager.GetAccountReportServiceUrlByGateWay(); - } - } + if (string.IsNullOrEmpty(_apiUrl)) + { + var resolved = GingerPlayEndPointManager.GetAccountReportServiceUrl(); + if (!string.IsNullOrWhiteSpace(resolved)) + { + _apiUrl = resolved.TrimEnd('/'); + } + } return _apiUrl; } set { if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) { - _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway + var baseVal = (value ?? string.Empty).TrimEnd('/'); + var gatewaySegment = GingerPlayEndPointManager.GetAccountReportServiceGateWay().Trim('/'); + var normalized = baseVal.EndsWith($"/{gatewaySegment}", StringComparison.OrdinalIgnoreCase) + ? baseVal + : $"{baseVal}/{gatewaySegment}"; + _apiUrl = normalized; + _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); } }
♻️ Duplicate comments (2)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
691-732: Nit: redundant local isSuccess (already addressed previously?)The local
isSuccessis initialized and returned but success/failure paths return early. Consider removing it and return false at the end. If you already fixed in another commit, ignore this.Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs (1)
604-606: Use safe values for log interpolation (optional)Action properties are interpolated directly; for resilience, prefer null-coalescing as used for SourcePath components.
- ExecutionProgressReporterListener.AddExecutionDetailsToLog(ExecutionProgressReporterListener.eExecutionPhase.End, "Action", string.Format("{0} (ID:{1}, ParentID:{2}, ParentActivityID: {3})", action.Description, action.Guid, action.ExecutionParentGuid, mCurrentActivity?.Guid), AR,SourcePath); + ExecutionProgressReporterListener.AddExecutionDetailsToLog(ExecutionProgressReporterListener.eExecutionPhase.End, "Action", string.Format("{0} (ID:{1}, ParentID:{2}, ParentActivityID: {3})", action?.Description ?? "N/A", action?.Guid, action?.ExecutionParentGuid, mCurrentActivity?.Guid), AR, SourcePath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs(3 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(5 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs(3 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionProgressReporterListener.cs(3 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-30T06:03:09.397Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4314
File: Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs:960-976
Timestamp: 2025-09-30T06:03:09.397Z
Learning: In GingerPlayDetails configuration mapping (file: Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs), both EnableAccountReportService and EnableHTMLReportService should be mapped from gingerPlayConfig.GingerPlayReportServiceEnabled as they represent the same underlying service.
Applied to files:
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.csGinger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.csGinger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-03-20T11:10:33.780Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:33.780Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-09-05T10:50:57.706Z
Learnt from: Maheshkale447
PR: Ginger-Automation/Ginger#4293
File: Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs:56-60
Timestamp: 2025-09-05T10:50:57.706Z
Learning: In AccountReportApiHandler.cs, the _isRunSetDataSent flag is correctly initialized to true by default. This flag acts as a circuit breaker where true allows operations to proceed and false blocks downstream operations after a RunSet send failure. The optimistic default (true) is necessary to allow the initial RunSet send attempt.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
🧬 Code graph analysis (4)
Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionLoggerManager.cs (1)
Ginger/GingerCoreNET/Run/RunListenerLib/ExecutionProgressReporterListener.cs (2)
ExecutionProgressReporterListener(33-185)AddExecutionDetailsToLog(97-183)
Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs (2)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (5)
AccountReportApiHandler(39-762)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)GetRunsetExecutionDataBySolutionIDFromCentralDB(598-631)GetRunsetExecutionDataByRunSetIDFromCentralDB(563-596)Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrl(81-115)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (2)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
Task(268-355)Task(358-514)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (4)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrlByGateWay(67-74)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (21)
List(503-531)List(533-561)List(633-661)Task(126-176)Task(178-200)Task(202-217)Task(219-242)Task(244-266)Task(268-290)Task(320-358)Task(361-393)Task(395-440)Task(443-475)Task(477-501)Task(663-689)Task(691-732)Task(734-761)AccountReportRunSet(95-120)AccountReportApiHandler(39-762)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
452-469: Stop using the action-name match to gate retry parsingLine 459 currently checks
ActionNamematch.Successbefore convertingTotalRetriesmatch; same forCurrentRetrymatch. When a failure log lacksDescription=(common with some plugins),ActionNamematch.Successis false, so you still executeConvert.ToInt32on an empty retry match, throwingFormatException. That exception bubbles out ofProcessBatch, the worker catches it, retries, and permanently drops the batch—so no execution logs reach the service. This was already flagged earlier and is still unresolved.Guard each conversion with the correct match (
TotalRetriesmatch/CurrentRetrymatch) and preferint.TryParseso missing data doesn’t explode the pipeline. For example:- int TotalRetries = 0; - var TotalRetriesmatch = Regex.Match(evt.RenderedMessage, @"Total Retries Configured= (.*?)\n"); - if (ActionNamematch.Success) - { - TotalRetries = Convert.ToInt32(TotalRetriesmatch.Groups[1].Value); - } - int CurrentRetry = 0; - var CurrentRetrymatch = Regex.Match(evt.RenderedMessage, @"Current Retry Iteration= (.*?)\n"); - if (ActionNamematch.Success) - { - CurrentRetry = Convert.ToInt32(CurrentRetrymatch.Groups[1].Value); - } + int TotalRetries = 0; + var totalRetriesMatch = Regex.Match(evt.RenderedMessage, @"Total Retries Configured= (.*?)\n"); + if (totalRetriesMatch.Success && int.TryParse(totalRetriesMatch.Groups[1].Value, out var parsedTotalRetries)) + { + TotalRetries = parsedTotalRetries; + } + int CurrentRetry = 0; + var currentRetryMatch = Regex.Match(evt.RenderedMessage, @"Current Retry Iteration= (.*?)\n"); + if (currentRetryMatch.Success && int.TryParse(currentRetryMatch.Groups[1].Value, out var parsedCurrentRetry)) + { + CurrentRetry = parsedCurrentRetry; + }Without this fix, execution logging remains brittle and can silently stall.
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (2)
693-734: Minor: Redundant variable initialization still present (from previous review).Line 695 initializes
isSuccess = false, which is only returned at line 733 when the guards at lines 696, 703, and 708 prevent entry into the main logic. This was flagged in a previous review and marked as addressed, but the variable remains.Apply this diff to remove the redundant variable:
public async Task<bool> SendExecutionLogToCentralDBAsync(string apiUrl, AccountReport.Contracts.RequestModels.ExecutionLogRequest executionLogRequest) { - bool isSuccess = false; if (string.IsNullOrWhiteSpace(apiUrl)) { Reporter.ToLog(eLogLevel.ERROR, "SendExecutionLogToCentralDBAsync: ApiUrl is required"); return false; } if (string.IsNullOrWhiteSpace(executionLogRequest.LogData)) { Reporter.ToLog(eLogLevel.DEBUG, "SendExecutionLogToCentralDBAsync: logData is empty, skipping"); return false; } if (restClient != null && _isRunSetDataSent) { executionLogRequest.LogId = LogId; string message = string.Format("execution log to Central DB (API URL:'{0}', Execution Id:'{1}', Instance Id:'{2}', Log Id:'{3}')", apiUrl, executionLogRequest.ExecutionId, executionLogRequest.InstanceId, executionLogRequest.LogId); try { string FinalAPIUrl = $"{apiUrl.TrimEnd('/')}/{SEND_EXECUTIONLOG}"; bool IsSuccessful = await SendExecutionLogRestRequestAndGetResponse(FinalAPIUrl, executionLogRequest).ConfigureAwait(false); if (IsSuccessful) { Reporter.ToLog(eLogLevel.DEBUG, $"Successfully sent {message}"); return true; } else { Reporter.ToLog(eLogLevel.ERROR, $"Failed to send {message}"); return false; } } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Exception when sending {message}", ex); return false; } } - return isSuccess; + return false; }
599-633: Critical: Same URL construction bug as above method.Line 603 has the identical issue—concatenating a constant containing
{solutionId}placeholder with the actual value.Apply this diff:
public string GetRunsetExecutionDataBySolutionIDFromCentralDB(Guid solutionId) { if (restClient != null) { - var path = $"{GET_RUNSET_EXECUTION_DATA_SOLUTION_ID}/{solutionId}/"; + var path = $"api/HtmlReport/GetRunsetsExecutionInfoBySolutionID/{solutionId}/"; RestRequest restRequest = new RestRequest(path, Method.Get); string message = string.Format("solution id : {0}", solutionId); try { RestResponse response = restClient.Execute(restRequest); if (!response.IsSuccessStatusCode) { if (response.StatusCode == System.Net.HttpStatusCode.NotFound) { - Reporter.ToLog(eLogLevel.DEBUG, $"Not found Execution Info againts solutionGuid : {solutionId} GetSolutionRunsetsExecutionInfo() :{response}"); + Reporter.ToLog(eLogLevel.DEBUG, $"Not found Execution Info against solutionGuid: {solutionId} GetSolutionRunsetsExecutionInfo(): {response}"); } else { - Reporter.ToLog(eLogLevel.ERROR, $"Error occurred during GetSolutionRunsetsExecutionInfo() :{response}"); + Reporter.ToLog(eLogLevel.ERROR, $"Error occurred during GetSolutionRunsetsExecutionInfo(): {response}"); } }Note: Also fixed typo "againts" → "against" (line 613).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Ginger/Ginger/Ginger.csproj(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(1 hunks)Ginger/GingerCoreNET/GingerCoreNET.csproj(1 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(5 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)Ginger/GingerCoreNETUnitTest/GingerCoreNETUnitTest.csproj(1 hunks)Ginger/GingerTest/GingerTest.csproj(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-09-05T10:50:57.706Z
Learnt from: Maheshkale447
PR: Ginger-Automation/Ginger#4293
File: Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs:56-60
Timestamp: 2025-09-05T10:50:57.706Z
Learning: In AccountReportApiHandler.cs, the _isRunSetDataSent flag is correctly initialized to true by default. This flag acts as a circuit breaker where true allows operations to proceed and false blocks downstream operations after a RunSet send failure. The optimistic default (true) is necessary to allow the initial RunSet send attempt.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
🧬 Code graph analysis (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrlByGateWay(67-74)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (21)
List(503-531)List(533-561)List(635-663)Task(126-176)Task(178-200)Task(202-217)Task(219-242)Task(244-266)Task(268-290)Task(320-358)Task(361-393)Task(395-440)Task(443-475)Task(477-501)Task(665-691)Task(693-734)Task(736-763)AccountReportRunSet(95-120)AccountReportApiHandler(39-764)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (2)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
Task(269-356)Task(359-525)
🔇 Additional comments (2)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
736-763: LGTM! Well-structured helper method.The
SendExecutionLogRestRequestAndGetResponsemethod properly handles the execution log POST request with appropriate error handling and logging. The implementation is consistent with the existing pattern in the codebase.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (1)
256-271: Ensure ElementName uniqueness across all batches
Current logic only checks for duplicates within the incoming batch; names generated in prior batches aren’t validated. Confirm or add a global duplicate-detection step spanning the entire POM.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
66-71: Fix ApiUrl setter: instantiate handler after computing final URL (prevents wrong base in REST calls)Handler is created with raw value before appending the gateway; RunSet update calls that rely on the handler base URL can hit the wrong endpoint.
Apply this diff:
- if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) - { - _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway - } + if (!string.IsNullOrWhiteSpace(value)) + { + var finalUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; // base + gateway + if (!string.Equals(_apiUrl, finalUrl, StringComparison.Ordinal)) + { + _apiUrl = finalUrl; + _accountReportApiHandler = new AccountReportApiHandler(_apiUrl); + } + } + else + { + _apiUrl = null; + _accountReportApiHandler = null; + }
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
487-500: Avoid Regex in hot path checks; use IndexOf/EqualsReplace Regex.IsMatch with simple string checks to reduce overhead in log processing.
Apply this diff:
- if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.IndexOf("Execution Ended for Run Set Operation", StringComparison.OrdinalIgnoreCase) >= 0 && + evt.RenderedMessage.IndexOf("Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0) { ExecutionErrorRequests.ErrorSource = "Pre Runset Operation Type"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted, true); } ... - if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.IndexOf("Execution Ended for Run Set Operation", StringComparison.OrdinalIgnoreCase) >= 0 && + evt.RenderedMessage.IndexOf("Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0) { ExecutionErrorRequests.ErrorSource = "Post Runset Operation Type"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted, true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrlByGateWay(67-74)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (21)
List(503-531)List(533-561)List(635-663)Task(126-176)Task(178-200)Task(202-217)Task(219-242)Task(244-266)Task(268-290)Task(320-358)Task(361-393)Task(395-440)Task(443-475)Task(477-501)Task(665-691)Task(693-734)Task(736-763)AccountReportRunSet(95-120)AccountReportApiHandler(39-764)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)
🔇 Additional comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
519-524: Non-update RunSet send is already wired
AccountReportExecutionLogger.SendRunsetExecutionDataToCentralDbTaskAsync (line 70) calls SendRunsetExecutionDataToCentralDBAsync withoutisUpdate, ensuring the RunSet start is sent before logs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
66-71: Fix ApiUrl setter: instantiate handler with final gateway URLCurrently creates AccountReportApiHandler with pre-final value, then appends gateway to _apiUrl. Instantiate with the final URL to avoid a mismatched base URL.
Apply:
- if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) - { - _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway - } + if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) + { + var finalUrl = string.IsNullOrWhiteSpace(value) + ? string.Empty + : $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; + _apiUrl = finalUrl; + _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); + }Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
74-80: Security: don’t disable TLS certificate validation by defaultRemoteCertificateValidationCallback => true allows MITM. Make it configurable (dev-only) and default to proper validation.
Apply minimal safe fix:
- var options = new RestClientOptions(apiUrl) + var options = new RestClientOptions(apiUrl) { ThrowOnAnyError = false, - RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true, FailOnDeserializationError = false, ThrowOnDeserializationError = false };If you must support insecure dev mode, gate it via config (e.g., appSetting "AllowInsecureTls" == true) and log a WARN when enabled. Based on learnings.
♻️ Duplicate comments (3)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
693-734: Remove redundant isSuccess variable and return directlyVariable is never set to true; use early returns.
Apply:
- public async Task<bool> SendExecutionLogToCentralDBAsync(string apiUrl, AccountReport.Contracts.RequestModels.ExecutionLogRequest executionLogRequest) + public async Task<bool> SendExecutionLogToCentralDBAsync(string apiUrl, AccountReport.Contracts.RequestModels.ExecutionLogRequest executionLogRequest) { - bool isSuccess = false; if (string.IsNullOrWhiteSpace(apiUrl)) { Reporter.ToLog(eLogLevel.ERROR, "SendExecutionLogToCentralDBAsync: ApiUrl is required"); return false; } @@ if (restClient != null && _isRunSetDataSent) { executionLogRequest.LogId = LogId; string message = string.Format("execution log to Central DB (API URL:'{0}', Execution Id:'{1}', Instance Id:'{2}', Log Id:'{3}')", apiUrl, executionLogRequest.ExecutionId, executionLogRequest.InstanceId, executionLogRequest.LogId); try { string FinalAPIUrl = $"{apiUrl.TrimEnd('/')}/{SEND_EXECUTIONLOG}"; bool IsSuccessful = await SendExecutionLogRestRequestAndGetResponse(FinalAPIUrl, executionLogRequest).ConfigureAwait(false); if (IsSuccessful) { Reporter.ToLog(eLogLevel.DEBUG, $"Successfully sent {message}"); return true; } else { Reporter.ToLog(eLogLevel.ERROR, $"Failed to send {message}"); - return false; + return false; } } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Exception when sending {message}", ex); - return false; + return false; } } - return isSuccess; + return false; }Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
480-493: Replace Regex.IsMatch in hot path with simple string checks (applies to both blocks)Use IndexOf and Equals to reduce overhead in per-event processing.
Apply:
- if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.IndexOf("Execution Ended for Run Set Operation", StringComparison.OrdinalIgnoreCase) >= 0 && + evt.RenderedMessage.IndexOf("Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0) { ExecutionErrorRequests.ErrorSource = "Pre Runset Operation Type"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted, true); } ... - if (Regex.IsMatch(evt.Level.DisplayName, @"^INFO$") && Regex.IsMatch(evt.RenderedMessage, @"Execution Ended for Run Set Operation.*Status= Failed", RegexOptions.Singleline)) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.IndexOf("Execution Ended for Run Set Operation", StringComparison.OrdinalIgnoreCase) >= 0 && + evt.RenderedMessage.IndexOf("Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0) { ExecutionErrorRequests.ErrorSource = "Post Runset Operation Type"; SetExecutionError(evt, ExecutionErrorRequests, isExecutionStarted, true); }
446-469: Precompile per-event regex patterns to cut allocationsSourcePath/Description/Retry/ActionId regexes are created per event; promote to static readonly Regex with RegexOptions.Compiled and reuse.
Would you like me to push a patch adding class-scope compiled patterns and replacing the Match calls?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(1 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(5 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-13T08:43:01.945Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4103
File: Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs:1252-1256
Timestamp: 2025-02-13T08:43:01.945Z
Learning: In GITSourceControl.cs, certificate validation should not be unconditionally disabled by returning true in FetchOptions.CertificateCheck. Instead, consider making it configurable and logging warnings when disabled.
Applied to files:
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-09-05T10:50:57.706Z
Learnt from: Maheshkale447
PR: Ginger-Automation/Ginger#4293
File: Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs:56-60
Timestamp: 2025-09-05T10:50:57.706Z
Learning: In AccountReportApiHandler.cs, the _isRunSetDataSent flag is correctly initialized to true by default. This flag acts as a circuit breaker where true allows operations to proceed and false blocks downstream operations after a RunSet send failure. The optimistic default (true) is necessary to allow the initial RunSet send attempt.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
🧬 Code graph analysis (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrlByGateWay(67-74)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (21)
List(503-531)List(533-561)List(635-663)Task(126-176)Task(178-200)Task(202-217)Task(219-242)Task(244-266)Task(268-290)Task(320-358)Task(361-393)Task(395-440)Task(443-475)Task(477-501)Task(665-691)Task(693-734)Task(736-763)AccountReportRunSet(95-120)AccountReportApiHandler(39-764)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
Task(269-356)Task(359-518)
🔇 Additional comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
184-207: Flush remaining items on Dispose — good additionSynchronous drain before cancel reduces log loss on shutdown.
442-446: Don’t mask Pre/Post RunSet operation errors when execution is startedEnsure the action-level branch doesn’t preempt Pre/Post operation checks.
Apply:
- else if (isExecutionStarted) + else if (isExecutionStarted && !isPreRunSetOperation && !isPostRunSetOperation) { - if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && evt.RenderedMessage.IndexOf("Action Execution Ended", StringComparison.OrdinalIgnoreCase) >= 0 && evt.RenderedMessage.IndexOf("Execution Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0) + if (evt.Level.DisplayName.Equals("INFO", StringComparison.Ordinal) && + evt.RenderedMessage.IndexOf("Action Execution Ended", StringComparison.OrdinalIgnoreCase) >= 0 && + evt.RenderedMessage.IndexOf("Execution Status= Failed", StringComparison.OrdinalIgnoreCase) >= 0)Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
439-501: Restore Pre/Post RunSet error handlingWhen a RunSet operation fails,
isExecutionStartedis still true, so thiselse if (isExecutionStarted)branch short-circuits the laterisPreRunSetOperation/isPostRunSetOperationblocks. As a result, we never capture those errors inExecutionErrorRequests, regressing the earlier fix. Gate the execution branch so it only runs when neither RunSet operation flag is active.Apply:
- else if (isExecutionStarted) + else if (isExecutionStarted && !isPreRunSetOperation && !isPostRunSetOperation)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
563-563: Add missing space after comma in method signature (nitpick).The method signature is missing a space after the comma between parameters.
Apply this diff:
- public string GetRunsetExecutionDataByRunSetIDFromCentralDB(Guid solutionId,Guid runSetId) + public string GetRunsetExecutionDataByRunSetIDFromCentralDB(Guid solutionId, Guid runSetId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs(5 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-13T08:43:01.945Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4103
File: Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs:1252-1256
Timestamp: 2025-02-13T08:43:01.945Z
Learning: In GITSourceControl.cs, certificate validation should not be unconditionally disabled by returning true in FetchOptions.CertificateCheck. Instead, consider making it configurable and logging warnings when disabled.
Applied to files:
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
📚 Learning: 2025-09-05T10:50:57.706Z
Learnt from: Maheshkale447
PR: Ginger-Automation/Ginger#4293
File: Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs:56-60
Timestamp: 2025-09-05T10:50:57.706Z
Learning: In AccountReportApiHandler.cs, the _isRunSetDataSent flag is correctly initialized to true by default. This flag acts as a circuit breaker where true allows operations to proceed and false blocks downstream operations after a RunSet send failure. The optimistic default (true) is necessary to allow the initial RunSet send attempt.
Applied to files:
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs
🧬 Code graph analysis (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
GingerPlayEndPointManager(27-265)GetAccountReportServiceUrlByGateWay(67-74)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (21)
List(503-531)List(533-561)List(635-663)Task(126-176)Task(178-200)Task(202-217)Task(219-242)Task(244-266)Task(268-290)Task(320-358)Task(361-393)Task(395-440)Task(443-475)Task(477-501)Task(665-691)Task(693-734)Task(736-763)AccountReportRunSet(95-120)AccountReportApiHandler(39-764)AccountReportApiHandler(67-90)AccountReportApiHandler(91-94)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (5)
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (5)
58-59: LGTM!The new constants follow the existing pattern and naming conventions.
567-568: LGTM!The URL path construction is correct, with no double slashes.
603-604: LGTM!The URL path construction is correct, with no double slashes.
693-734: Verify the intended scope of LogId assignment.Line 710 assigns the instance-level
LogId(initialized once at line 65) to everyExecutionLogRequest. This means all execution logs sent through the sameAccountReportApiHandlerinstance will share the sameLogId.Please confirm this is the intended design:
- If logs should be grouped by handler instance: current implementation is correct
- If each batch or request should have a unique
LogId: the field should be generated per call instead
736-763: LGTM!The refactoring to accept
ExecutionLogRequestdirectly makes the code more consistent and cleaner. The logic is correct.
This was referenced Oct 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
Chores