Skip to content

Added trace information - #36

Open
diogopaulo wants to merge 4 commits into
mainfrom
bug-printtopdf-advanced-torest-403
Open

Added trace information#36
diogopaulo wants to merge 4 commits into
mainfrom
bug-printtopdf-advanced-torest-403

Conversation

@diogopaulo

Copy link
Copy Markdown
Contributor

Added tracing information to the code.

This information appears in the traces and enables show details 👁️

image

@GavinMeredithBehr GavinMeredithBehr 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.

Does the trace logging make any noticeable effect on PDF generation time? Or is it minimal overhead to include everywhere?

The code looks good, just took a little time to check as there seemed to be a bit of code refactoring as well as addition of tracing.

Final comment is, does it make sense to include the tracing in the NullLogger?

Nothing above blocks the review just curious on the above questions as I don't believe performance will be massively impacted I just wondered if we knew how noticeable it might be.

@diogopaulo

Copy link
Copy Markdown
Contributor Author

Does the trace logging make any noticeable effect on PDF generation time? Or is it minimal overhead to include everywhere?

From my tests the impact is neglectable.

Final comment is, does it make sense to include the tracing in the NullLogger?

That is a question that I was also wondering if it makes sense to have this on the key methods, or if it makes sense to have in everything.

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