Skip to content

D52125 Execution Logger Fix#4321

Merged
Maheshkale447 merged 5 commits into
Releases/Official-Releasefrom
BugFix/ExecutionLog
Oct 9, 2025
Merged

D52125 Execution Logger Fix#4321
Maheshkale447 merged 5 commits into
Releases/Official-Releasefrom
BugFix/ExecutionLog

Conversation

@prashelke

@prashelke prashelke commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

Thank you for your contribution.
Before submitting this PR, please make sure:

  • PR description and commit message should describe the changes done in this PR
  • Verify the PR is pointing to correct branch i.e. Release or Beta branch if the code fix is for specific release , else point it to master
  • Latest Code from master or specific release branch is merged to your branch
  • No unwanted\commented\junk code is included
  • No new warning upon build solution
  • Code Summary\Comments are added to my code which explains what my code is doing
  • Existing unit test cases are passed
  • New Unit tests are added for your development
  • Sanity Tests are successfully executed for New and Existing Functionality
  • Verify that changes are compatible with all relevant browsers and platforms.
  • After creating pull request there should not be any conflicts
  • Resolve all Codacy comments
  • Builds and checks are passed before PR is sent for review
  • Resolve code review comments
  • Update the Help Library document to match any feature changes

Summary by CodeRabbit

  • New Features

    • Added account-report gateway support used when composing logging endpoints.
  • Bug Fixes

    • Log uploads now route correctly through gateway-based deployments.
    • Trimmed trailing newlines to a single line per entry for cleaner output.
  • Reliability

    • Logging preserves event properties, handles enqueue and processing errors gracefully, and is more resilient to exceptions.
    • Invalid logging configurations now fall back to safe defaults and are reported at DEBUG (less noisy).

@coderabbitai

coderabbitai Bot commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds a gateway accessor to GingerPlayEndPointManager and integrates it into HttpLogAppender's ApiUrl composition; refactors HttpLogAppender Append/ProcessQueue/activation behavior to snapshot event properties, handle exception-bearing events, reduce trailing newlines, and downgrade several error logs to debug.

Changes

Cohort / File(s) Summary of Changes
Endpoint management
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs
Added public static method GetAccountReportServiceGateWay() returning ACCOUNT_REPORT_SERVICE_URL.
Logging / HTTP appender
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs
ApiUrl setter: assigns pre-gateway _apiUrl, initializes _accountReportApiHandler for it, then appends gateway from GingerPlayEndPointManager.GetAccountReportServiceGateWay() to form the final _apiUrl. ActivateOptions and validation logs: several invalid-config logs changed from ERROR to DEBUG and defaults enforced. Append(LoggingEvent): snapshots properties, creates a new event (using exception-aware ctor when needed), copies properties, enqueues with concurrency/queue-state guards and DEBUG-level handling on failures. ProcessQueue: uses a single trailing newline per event (was two) and downgrades multiple failure/ retry logs to DEBUG. Error handling and disposal paths log failures at DEBUG instead of ERROR.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller
    participant HttpLogAppender
    participant EndPointMgr as GingerPlayEndPointManager
    participant APIHandler as AccountReportApiHandler

    Note over Caller,HttpLogAppender: ApiUrl setter (compose with gateway)
    Caller->>HttpLogAppender: set ApiUrl(value)
    HttpLogAppender->>HttpLogAppender: _apiUrl = value (pre-gateway)
    HttpLogAppender->>APIHandler: init/ensure handler for pre-gateway _apiUrl
    HttpLogAppender->>EndPointMgr: GetAccountReportServiceGateWay()
    EndPointMgr-->>HttpLogAppender: ACCOUNT_REPORT_SERVICE_URL
    HttpLogAppender->>HttpLogAppender: _apiUrl = pre-gateway + gateway
    HttpLogAppender->>APIHandler: ensure handler for composed _apiUrl

    Note over Caller,HttpLogAppender: Append flow (snapshot props, exception-aware)
    Caller->>HttpLogAppender: Append(LoggingEvent e)
    HttpLogAppender->>HttpLogAppender: snapshot Properties from e
    alt e has Exception
        HttpLogAppender->>HttpLogAppender: create new event with exception-aware ctor
    else
        HttpLogAppender->>HttpLogAppender: create new event with standard ctor
    end
    HttpLogAppender->>HttpLogAppender: copy snapshot properties to new event
    HttpLogAppender->>APIHandler: enqueue rendered event (POST with single "\n")
    APIHandler-->>HttpLogAppender: send result (success / retry / fail logged at DEBUG)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • Maheshkale447

Poem

🐇 I hopped in code, a tiny feat,
Added a gateway, neat and sweet.
Logs now copied, exceptions caught,
One newline left—no extra spot.
Hop, build, report — the trail's complete.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description currently only repeats the repository’s generic checklist template without providing any actual summary of the implemented fixes, rationale, testing details, or confirmation of completed items, so it fails to describe the changes made in this PR. Please replace the placeholder checklist with a clear description of the changes, including the purpose of the Execution Logger fix, details of the code modifications, results of relevant tests, and which checklist items have been completed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title “D52125 Execution Logger Fix” concisely indicates that the PR addresses an issue with the execution logging functionality and aligns directly with the extensive changes to the HttpLogAppender and related logging endpoints, making the primary change clear to reviewers at a glance.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BugFix/ExecutionLog

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4373452 and e29321c.

📒 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-276)
  • GetAccountReportServiceGateWay (87-90)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
  • Reporter (28-357)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/log4netLib/HttpLogAppender.cs (1)

41-49: Instantiate handler after appending the gateway URL
In ApiUrl’s setter, build the full URL first, then create the handler. For example, replace the current block with:

_apiUrl = $"{value}{GingerPlayEndPointManager.GetAccoutReportServiceGateWay()}";  
_accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl)  
    ? null  
    : new AccountReportApiHandler(_apiUrl);  

This ensures the RestClient in AccountReportApiHandler is initialized with the correct full endpoint.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c78046 and 5d57473.

📒 Files selected for processing (2)
  • Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (1 hunks)
  • Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/External/GingerPlay/GingerPlayEndPointManager.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
  • GingerPlayEndPointManager (27-276)
  • GetAccoutReportServiceGateWay (87-90)
🔇 Additional comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)

213-213: Verify the log entry formatting change.

The trailing newline has been reduced from two newlines to one, which will reduce spacing between log entries. Ensure this formatting change aligns with the expected log output format and doesn't impact log readability or parsing.

Comment thread Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/log4netLib/HttpLogAppender.cs (1)

43-48: Fix the URL composition order.

The AccountReportApiHandler is being initialized with an incomplete URL. Line 46 creates the handler using _apiUrl (which equals value), but line 47 then appends the gateway segment to _apiUrl. This means the handler is created with the wrong URL.

Additionally, there's no slash handling when concatenating value and the gateway segment, which could result in malformed URLs like https://example.comginger-report.

Apply this diff to fix the logic:

 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
+    // Compose the final URL with gateway before creating the handler
+    var gateway = GingerPlayEndPointManager.GetAccountReportServiceGateWay();
+    var baseUrl = value?.TrimEnd('/') ?? string.Empty;
+    var gatewaySegment = gateway?.TrimStart('/') ?? string.Empty;
+    _apiUrl = string.IsNullOrWhiteSpace(baseUrl) ? string.Empty : $"{baseUrl}/{gatewaySegment}";
+    _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl);
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d57473 and 623d7c4.

📒 Files selected for processing (2)
  • Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (1 hunks)
  • Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/External/GingerPlay/GingerPlayEndPointManager.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
Ginger/GingerCoreNET/External/GingerPlay/GingerPlayEndPointManager.cs (2)
  • GingerPlayEndPointManager (27-276)
  • GetAccountReportServiceGateWay (87-90)
🔇 Additional comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)

213-213: LGTM!

Reducing the trailing newlines from two to one improves log readability by reducing excessive whitespace between log entries.

Comment thread Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs Outdated
@Maheshkale447
Maheshkale447 merged commit ce2f7a1 into Releases/Official-Release Oct 9, 2025
11 checks passed
@Maheshkale447
Maheshkale447 deleted the BugFix/ExecutionLog branch October 9, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants