Add possibility to fetch all logged warning programmatically, see #76 #489

syjer · 2020-05-25T11:42:13Z

Hi @danfickle , this PR is still a work in progress, but the main functionality should be here.

As you described in the issue #76 (#76 (comment)) , adding a way to fetch all logged values programmatically is quite a nice feature, this PR enable that.

I still need to do:

check if the cleanup is right in all cases
expose the Diagnostic values (add getters)
maybe add an option to enable the recording of log in the builder, instead of doing it by default
tests

What do you think ? :)

danfickle · 2020-05-27T13:22:42Z

Hello @syjer,

I was just thinking, why not allow the user to provide a Consumer in the builder? That way they can do whatever they like with them rather than just an ArrayList.

Eventually, I think of two things with "logging":

Log with enum constants (convertible to strings), so the user can programmatically determine what is being logged without resorting to ugly string comparison.
The ability to throw a poison pill exception on any selected log callback. This would allow for example some users to stop the rendering on any load failure while others would want a more tolerant approach. Currently the user can not configure what happens.

Of course, 1 is a lot of work!

syjer · 2020-05-27T13:32:32Z

hi @danfickle

I was just thinking, why not allow the user to provide a Consumer in the builder? That way they can do whatever they like with them rather than just an ArrayList.

You are right! Nice idea, it will simplify some logic too.

1: yep, I'll see how many different logging messages are present. It may be a bit mechanical as a work, but I guess it will pay off.

2: agree, I guess using the builder approach + enum, we can ensure it will be possible to let the user block the execution as he want.

I'll try to update the code for next week :)

syjer · 2020-06-04T11:24:06Z

switched to the Consumer approach.

Next I'll try to switch to the enum variant

danfickle · 2020-06-13T06:01:42Z

openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/XRLog.java

    }

+    public enum LogMessageId {


I think LogMessageId will be big enough to warrant its own file.

Also, I'm worried that since we have a lot of code paths that are not tested, that we will get into trouble with mismatching argument counts for the message format. One way to alleviate this problem would be to have an message enum for each arg count and a corresponding method which accepted that enum and the correct number of args. Then we would make the enums implement a common interface, which is what the user would see. Interested in your feedback.

agree about the new file.

About the mismatching count of parameters/message, I agree we should be able to statically prevent the misuse. I'm wondering if there is an alternative way to implement it, losing the single enum type is unfortunate.

done, I think it will work :)

danfickle · 2020-06-13T06:04:22Z

openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/ThreadCtx.java

+		if (consumer != null) {
+			consumer.accept(new Diagnostic(where, level, msg, th));
+		}
+	}

 	public static void cleanup() {
 		data.remove();


I think we have to diagnosticConsumer.remove() here to avoid zombie data.

I see on second thoughts that you have dealt with it using closeable of your own in apply method. Your method is probably cleaner.

openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/ThreadCtx.java

danfickle · 2020-06-13T06:16:11Z

I've added a few inline comments, but I'm generally pleased with the look of this. My thinking was very similar.

syjer · 2020-06-13T11:42:53Z

I've pushed some additional code following your feedback, I think it's beginning to look decent :).

Btw I've added a FIXME comment here https://github.com/danfickle/openhtmltopdf/pull/489/files#diff-1240d78636f3f4023a60d044cd8184eaR97 , as I'm still not sure how to bridge the new Diagnostic format to the logger interface in an elegant way.

danfickle · 2020-06-22T14:03:12Z

You have been busy! In regards to the legacy logging situation, I think the core problem is that java.util.logging where the log messages end up by default, does not support parameters, just a single string, which is a performance waste if a log message is not ultimately used. Fortunately, slf4j , which most people probably already have on the classpath, does support parameters.

Therefore, I'm thinking that we work on the slf4j logger module (to use the new parameterised log messages, will require a new interface) and recommend that people use it. Of course, users can continue to use j.u.l at a performance penalty.

P.s. if we stick with an old version of slf4j, users can exclude our copy via maven, as it is supposedly binary backwards compatible.

syjer · 2020-06-23T07:25:26Z

yep. I'm organizing the code to have 0 overhead for slf4j.

I'm hoping to complete the PR this week :)

syjer · 2020-06-23T11:50:01Z

I've pushed the diagnostic bridging implementation:

A default one: 69fb5f5#diff-14a42331e2299f386db6d8cc423d57c8R39

And a custom one for slf4j:

69fb5f5#diff-6da5c18b096d790496d094f126d941f1R58

syjer · 2020-06-29T14:47:19Z

I think I got everything now :).

syjer · 2020-07-10T11:18:37Z

after the merge of #507 I need to resolve some conflicts, I'll do it today :)

danfickle · 2020-07-10T11:38:56Z

One thing I've just noticed, the buildpdfrenderer method in pdfrendererbuilder is public, and used by users wanting the pddocument for post processing. Therefore, it needs to behave the same as run for the consumer.

Other than that it all looks excellent online, but I will have a look on my ide after you fix merge issues.

P.s. I appreciate the massive amount of work in this pr, so another big thanks.

syjer · 2020-07-10T11:49:02Z

One thing I've just noticed, the buildpdfrenderer method in pdfrendererbuilder is public, and used by users wanting the pddocument for post processing. Therefore, it needs to behave the same as run for the consumer.

I guess I forgot to cover that case. I'll update there too.

P.s. I appreciate the massive amount of work in this pr, so another big thanks.

You're welcome :)

…diagnostic messages

syjer · 2020-07-10T12:40:07Z

In the last commit e705452 I've propagated the Consumer inside the *Renderer and wired up the closing of the consumer inside their close() method.

With that I think we have covered all the cases?

danfickle · 2020-07-11T09:43:40Z

Yes, I think that is everything. Thanks again.

Making slf4j-api provided scope should reduce dependency hell for end users as almost all will already have slf4j-api on their classpath. Also added a couple of manual tests to make sure module is working as expected.

Works brilliantly.

syjer · 2020-07-11T12:22:23Z

thank you for the review and merging 👍 .

I'll most likely do another round of cleanup as I think there is still some old unused code and pattern that can be improved using java8 api.

syjer changed the title ~~Add possibility to fetch all logged warning, see #76~~ Add possibility to fetch all logged warning programmatically, see #76 May 25, 2020

danfickle reviewed Jun 13, 2020

View reviewed changes

openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/ThreadCtx.java Outdated Show resolved Hide resolved

syjer marked this pull request as ready for review June 29, 2020 14:43

syjer added 2 commits July 10, 2020 14:19

Refactor logging mechanism, allow the possibity to listen and handle …

109ad5a

…diagnostic messages

propagate the diagnostic consumer inside the *Renderer

e705452

danfickle merged commit 3a3ad28 into danfickle:open-dev-v1 Jul 11, 2020

danfickle added a commit that referenced this pull request Jul 11, 2020

#489 Add simple test for diagnostic consumer.

e44fb86

Works brilliantly.

danfickle mentioned this pull request Jul 25, 2020

Upload to maven central via bintray. #7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to fetch all logged warning programmatically, see #76 #489

Add possibility to fetch all logged warning programmatically, see #76 #489

syjer commented May 25, 2020

danfickle commented May 27, 2020

syjer commented May 27, 2020

syjer commented Jun 4, 2020

danfickle Jun 13, 2020

syjer Jun 13, 2020

syjer Jun 13, 2020

danfickle Jun 13, 2020

danfickle Jun 13, 2020

danfickle commented Jun 13, 2020

syjer commented Jun 13, 2020 •

edited

Loading

danfickle commented Jun 22, 2020

syjer commented Jun 23, 2020

syjer commented Jun 23, 2020

syjer commented Jun 29, 2020

syjer commented Jul 10, 2020 •

edited

Loading

danfickle commented Jul 10, 2020

syjer commented Jul 10, 2020

syjer commented Jul 10, 2020 •

edited

Loading

danfickle commented Jul 11, 2020

syjer commented Jul 11, 2020

Add possibility to fetch all logged warning programmatically, see #76 #489

Add possibility to fetch all logged warning programmatically, see #76 #489

Conversation

syjer commented May 25, 2020

danfickle commented May 27, 2020

syjer commented May 27, 2020

syjer commented Jun 4, 2020

danfickle Jun 13, 2020

Choose a reason for hiding this comment

syjer Jun 13, 2020

Choose a reason for hiding this comment

syjer Jun 13, 2020

Choose a reason for hiding this comment

danfickle Jun 13, 2020

Choose a reason for hiding this comment

danfickle Jun 13, 2020

Choose a reason for hiding this comment

danfickle commented Jun 13, 2020

syjer commented Jun 13, 2020 • edited Loading

danfickle commented Jun 22, 2020

syjer commented Jun 23, 2020

syjer commented Jun 23, 2020

syjer commented Jun 29, 2020

syjer commented Jul 10, 2020 • edited Loading

danfickle commented Jul 10, 2020

syjer commented Jul 10, 2020

syjer commented Jul 10, 2020 • edited Loading

danfickle commented Jul 11, 2020

syjer commented Jul 11, 2020

syjer commented Jun 13, 2020 •

edited

Loading

syjer commented Jul 10, 2020 •

edited

Loading

syjer commented Jul 10, 2020 •

edited

Loading