Include registration error in the log

[khash]

If registration fails, the error is not logged which includes useful information

[kamikazechaser]

@khash Are you able to update this PR?

[khash]

@kamikazechaser done

[kamikazechaser]

@khash See my comment on the review.

[khash]

@khash See my comment on the review.

I'm sorry but I don't see any PR reviews or file comment on this.

[kamikazechaser]

Undo this removal:

s.logger.Debugf("Writing entries %v", entries)

[khash]

Undo this removal:

s.logger.Debugf("Writing entries %v", entries)

As per the commit message, this log fires every 5 seconds and fills up a lot of the logs with the same message. what value does this add at this frequency?

[kamikazechaser]

It is for whoever wants to run the library with debug mode. We generally don't remove anything from this library unless there is a very good reason to do so.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 66.30%. Comparing base (4f00f52) to head (f5c5b34).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
periodic_task_manager.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
- Coverage   67.13%   66.30%   -0.84%     
==========================================
  Files          29       29              
  Lines        4300     5728    +1428     
==========================================
+ Hits         2887     3798     +911     
- Misses       1135     1651     +516     
- Partials      278      279       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[khash]

It is for whoever wants to run the library with debug mode. We generally don't remove anything from this library unless there is a very good reason to do so.

While I agree that this is a debug message, I also know that this library is not the only thing that runs in my application. As such, logging the entire payload of a job, every 5 seconds, when there are many other application logs, makes debugging very difficult exactly at the time you'd want to look at the flow of a payload of a job from the framework into the job itself.

I always use trace for that purpose not debug, but if the used logging library in asynq does't support a lower level of logging than debug, I would suggest logging only once of every few times.

This is a comment I would make in general about logging in infrastructure (or middleware in some lingo) libraries as well. The reason for having a debug log is to debug the code. In this case, it means debugging the asynq code, as in most cases, the payload can be dumped in other places (within the application code) as well.

However, debug is a log level across the entire application and in 99.9% of the cases, is used to debug the application, not the infrastructure part of it. Therefore having a verbose debug log in places like db driver or async queue library is going to permanently add a lot of noise, without much value to the entire application log, exactly when you'd want to focus on application bugs. Another way could be having a flag to explicitly enable logging in the library itself, which I've seen in other middleware libraries, like Echo.

If you feel strongly about this issue, feel free to dismiss the PR, I'll continue using the patched version.

[kamikazechaser]

If you feel strongly about this issue

Not at all. I looked at other instances of how debug is being used and I agree with you; this debugf line should not exist given how asynq is being used today with potentially 1000s of entries. I believe the original author did not anticipate this case.