Conversation
Why these changes are being introduced: Our current logging doesn't capture much information. (It usually just says 'N/A'.) While DSpace errors are not useful, we should still probably try to capture what's there. Relevant ticket(s): N/A How this addresses that need: That adds some Copilot-suggested changes to enhance our logging in the DSpace publication results job. Side effects of this change: None.
There was a problem hiding this comment.
Pull request overview
Enhances DSpace publication results logging by extracting more detail from DSS error payloads so errors are less frequently “N/A” and more actionable.
Changes:
- Replace single-field DSS error logging with a formatted, multi-field error summary (ErrorInfo/DSpaceResponse/ExceptionMessage/ExceptionTraceback).
- Add traceback formatting to keep stack traces readable (first few non-blank lines).
- Extend the job test to assert that DSS error payload details appear in the returned
results[:errors].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/jobs/dspace_publication_results_job.rb | Adds format_dss_error + format_traceback and uses them when ResultType == 'error' to improve logged/error-report detail. |
| test/jobs/dspace_publication_results_job_test.rb | Adds an assertion ensuring ErrorInfo details are surfaced in the collected errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def format_dss_error(body) | ||
| details = [] | ||
| details << "ErrorInfo: #{body['ErrorInfo']}" if body['ErrorInfo'].present? | ||
| details << "DSpaceResponse: #{body['DSpaceResponse']}" if body['DSpaceResponse'].present? | ||
| details << "ExceptionMessage: #{body['ExceptionMessage']}" if body['ExceptionMessage'].present? | ||
|
|
||
| traceback = format_traceback(body['ExceptionTraceback']) | ||
| details << "ExceptionTraceback: #{traceback}" if traceback.present? | ||
|
|
||
| return 'No error details provided by DSS' if details.empty? | ||
|
|
||
| details.join(' | ') |
There was a problem hiding this comment.
format_dss_error interpolates raw DSS payload fields (including DSpaceResponse and ExceptionTraceback) directly into logs/emails. These fields can be extremely large and/or contain newlines, which can blow up log volume and make the results email hard to read. Consider truncating each field to a reasonable length (and normalizing whitespace/newlines) before joining, while still keeping enough context for debugging.
| 'appropriate action.' | ||
|
|
||
| # DSS error details are surfaced from error payload | ||
| assert results[:errors].any? { |e| e.include?('ErrorInfo: Stuff broke') } |
There was a problem hiding this comment.
This assertion will fail with an unhelpful default message and only checks for one substring anywhere in results[:errors]. Consider asserting against the specific error entry generated by this scenario (or at least providing an explicit failure message / also checking for ExceptionMessage/traceback formatting) so the test better pins the expected logging format.
| assert results[:errors].any? { |e| e.include?('ErrorInfo: Stuff broke') } | |
| dss_error = results[:errors].find { |e| e.include?('ErrorInfo: Stuff broke') } | |
| assert_not_nil dss_error, 'Expected DSS error with "ErrorInfo: Stuff broke" to be surfaced in results[:errors]' | |
| assert_includes dss_error, 'ErrorInfo: Stuff broke' | |
| assert_includes dss_error, 'ExceptionMessage: 500 Server Error: Internal Server Error' | |
| assert_includes dss_error, 'ExceptionTraceback: Full unformatted stack trace of the Exception' |
| traceback.split("\n") | ||
| else | ||
| [] | ||
| end | ||
|
|
There was a problem hiding this comment.
format_traceback only splits strings on "\n". If DSS sends Windows line endings (\r\n), the resulting lines can retain \r and produce messy output. Consider splitting with a regex like /\r?\n/ and/or stripping each line before joining.
| traceback.split("\n") | |
| else | |
| [] | |
| end | |
| traceback.split(/\r?\n/) | |
| else | |
| [] | |
| end | |
| lines = lines.map { |line| line.to_s.strip } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why these changes are being introduced:
Our current logging doesn't capture much
information. (It usually just says 'N/A'.) While
DSpace errors are not useful, we should still
probably try to capture what's there.
Relevant ticket(s):
N/A
How this addresses that need:
That adds some Copilot-suggested changes to
enhance our logging in the DSpace publication
results job.
Side effects of this change:
None.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO