Skip to content

Conversation

@neithanmo
Copy link
Collaborator

This PR improves error reporting and logging in our middleware components(for attestation) without changing their existing behavior. It addresses issues where errors in middleware functions were neither reported in response bodies(just an error code) nor logged via tracing, making debugging unnecessarily difficult.

Key Changes

  • Added structured error responses using ErrorResponse for better client feedback
  • Implemented detailed tracing logs for error conditions, however, the current interface does not treat them as errors but as optional state changes.
  • Maintained backward compatibility with the existing middleware chain behavior
  • Added explicit error/warning messages when signers are missing but attestation is requested

Fixes #619 - Error results from middleware functions not reported in logs or responses

- Add detailed error logging in attestation middleware
- Enhance signer middleware with better error reporting
- Improve error responses with structured JSON format
- Maintain backward compatibility with existing behavior
- Add tests for error handling scenarios

This change addresses debugging challenges by adding proper tracing
while preserving the original middleware behavior.
@neithanmo neithanmo force-pushed the feat/middleware_logging branch from 7fee8e5 to da3fc4f Compare March 20, 2025 13:28
@coveralls
Copy link

coveralls commented Mar 20, 2025

Pull Request Test Coverage Report for Build 13979872372

Details

  • 3 of 9 (33.33%) changed or added relevant lines in 3 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 76.258%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/service/src/middleware/attestation_signer.rs 1 2 50.0%
crates/service/src/middleware/attestation.rs 0 5 0.0%
Files with Coverage Reduction New Missed Lines %
crates/service/src/middleware/attestation.rs 1 88.18%
crates/watcher/src/lib.rs 4 92.71%
Totals Coverage Status
Change from base Build 13955914723: -0.07%
Covered Lines: 8711
Relevant Lines: 11423

💛 - Coveralls

suchapalaver
suchapalaver previously approved these changes Mar 20, 2025
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Nice! I take it this came out of the experience of working on testing patterns?

Yes, I am having trouble to get test scripts running and just get :

* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
< connection: close
< content-length: 0
< date: Thu, 20 Mar 2025 21:07:00 GMT

which is not that helpful. I am trying to submit a receipt.
I will need to do similar changes in the tap receipt handler as well, but that would be in another PR

@neithanmo neithanmo marked this pull request as ready for review March 20, 2025 13:38
Copy link
Collaborator

@TypeLevelConsoli TypeLevelConsoli left a comment

Choose a reason for hiding this comment

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

great, I'm all in for structured errors

@neithanmo
Copy link
Collaborator Author

great, I'm all in for structured errors

Thanks! @TypeLevelConsoli for suggestion, indeed, that way is clearer

@neithanmo neithanmo force-pushed the feat/middleware_logging branch from 4b533de to b7183b2 Compare March 20, 2025 21:25
@neithanmo neithanmo merged commit 40c696c into main Mar 20, 2025
10 checks passed
@neithanmo neithanmo deleted the feat/middleware_logging branch March 20, 2025 22:07
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.

middleware errors not logged

5 participants