Could please re-run phuslu/slog with latest commit

[phuslu]

Many thanks for adding phuslu/slog to your benchmark.
The current phuslu/slog is very rough, it only passed slogtest and focus on common performance -- logging/simple/simplesource suites in your repo.
After review&learn your other test cases, I understood that phuslu/slog has poor performance in vary/variable groups cases.

So I tried to improve phuslu/slog performance today, in master branch https://github.com/phuslu/log/commits/master/

Could you please give a chance to verify it? Very appreciate for your works!

[phuslu]

sorry, it's github.com/phuslu/log@6e8a3d0f36a9820bde0f0bd2862abdf33aea4292

the v1.0.93 is old one for phuslu/log#69

[madkins23]

I'm having an odd issue with some of the benchmark tests (With Group Attributes and With Group Key Values) with your code up through v1.0.93. I'm not sure it's in your code, thus far I can't duplicate it outside of the benchmark suite. These two tests take inordinate amounts of time to run for some reason, and sometimes they crash. I'm still looking into this, it may be my issue. Right now it's looking like delays relate to one or more of the sync.Pool object's you're using, I just can't figure out what triggers it.

That being said, I was able to push that version and it runs to completion in Github Actions so that's what is currently showing. The numbers are just not any better for those two tests.

When I pull the tip of your master branch I get a different error (during the verification tests):

    utility.go:47: 
        	Error Trace:	/home/marc/work/go/src/github.com/madkins23/go-slog/verify/tests/utility.go:47
        	            				/home/marc/work/go/src/github.com/madkins23/go-slog/verify/tests/slogtest.go:186
        	Error:      	Received unexpected error:
        	            	invalid character '}' after top-level value: '{"time":"2024-05-02T06:43:35.148167403-07:00","level":"INFO","msg":"This is a message"}}
        	            	'
        	Test:       	TestVerifyPhusluSlog/TestWithGroupEmpty

The test is:

logger.WithGroup("group1").WithGroup("group2").Info(message, slog.Group("subGroup"))

A second test fails in a similar manner, probably with the sequence:

logger.WithGroup("group1").WithGroup("group2").Info(message)

The tip of your branch does have better numbers for the two tests that have really long numbers at v1.0.93 (and before). So if the invalid character '}' after top-level value is fixed I can push that version. It won't generate new numbers until that is fixed due to the verification error.

[phuslu]

Thanks again! Let me try to continue my improvement.

[phuslu]

Please ignore my request now(don't update your benchmark code&resuslts). I gave up current approach and will totally rewrite phuslu/slog handler. thanks again!

[phuslu]

Hi @madkins23 , thank you for your help and guide before. Now phuslu/slog is ready for your review/test now(again 😄 )

Since my latest commit in phuslu/log@4daa79c
The go-slog verify/bench results look good currently
verify: https://github.com/phuslu/log/actions/runs/8934767267/job/24542167109
bench: https://github.com/phuslu/log/actions/runs/8934767267/job/24542166791

[madkins23]

Pushed. Looks good.

warning.GroupAttrMsgTop was created for your handler (this is pretty normal, new handler -> new idiosyncratic behavior). It was really peculiar and took a while to nail down. Since you've reworked the group code the issue apparently went away and you can remove the warning from the test.

There's a warning.StringAny being triggered despite not showing up in the test file. Something for me to look into.

[phuslu]

thanks a lot, I will try to improve in later days then release a new version/tag -- But I supposed there will no significantly performance improvement, just code styles.

thanks again for make phuslu/slog become reality

[phuslu]

According benchmark in https://madkins23.github.io/go-slog/handler/PhusluSlog.html , Currently phuslu/slog still has two performance shortcomings: Disabled and With Attrs Key Values

1. I tried a tweak "Disabled" performance to match phsym/zeroslog score.
2. For With Attrs Key Values, I checked the code then found it's hard to to improve it because I have to check the empty here while phsym/zeroslog seems didn't.

[phuslu]

For StringAny warnings, (If my understanding is correct), maybe I need change e.Any(a.Key, value.Any()) to e.Interface(a.Key, value.Any()) here:

https://github.com/phuslu/log/blob/fe42011085caece4d66b9b336b40e236cdaeafe5/slog.go#L55

[madkins23]

The StringAny warning is unique to phuslu/slog (the Usage buttons on the warnings page shows which handlers throw each warning). I'm not sure how important it is (keep in mind, I just find these things, I'm not the authority on this stuff), but none of the other handlers seem to do it. On the other hand, it may never come up in normal usage. You might wish to look at the relevant code for other existing handlers.

[phuslu]

thanks for explanation, I added a commit on phuslu/log tip to fix StringAny warnings

Historical reason is that phuslu/log Interface function matches zerolog implementation.

[phuslu]

I believe the StringAny warning is fixed and I released/tagged phuslu/log v1.0.94
I suppose that this is the "final version" of phuslu/slog, so I would appreciate it if you could update latest go-slog results based on phuslu/log v1.0.94.

[madkins23]

OK, should be done.

FYI: The go-slog server pages are rebuilt every Sunday (I think). Weekly, anyway. If you tag your code it should automatically get pulled in and recalculated (and the dependencies updated in the go-slog code base). You can always ask for an update with an issue like you did here, but if I'm out of touch or something the update should happen (eventually) anyway as long as you tag it.

[phuslu]

understood.

Currently I have a real case to demonstrate phuslu/log performance, thanks again.