Add documentation on the internals of cache_log_message - #7
Conversation
Sorry, something went wrong.
rousskov
left a comment
There was a problem hiding this comment.
Thank you for adding these hints for Squid developers.
Sorry, something went wrong.
| To instrument a message, one needs to: | ||
| 1. make a note of the value of variable `DebugMessageIdUpperBound` in file `src/debug/Messages.h` | ||
| 1. increment it by 1 | ||
| 1. add a line corresponding to the message in `doc/debug-messages.dox` |
There was a problem hiding this comment.
That file is generated automatically by scripts/source-maintenance.sh. Developers should not edit it by hand. We should add a corresponding comment to that file (but that is obviously outside this PR scope). Developers should run scripts/source-maintenance.sh (and recommending that step is in this PR scope).
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. I didn't know about it. This is why I rote this PR in the first place
Sorry, something went wrong.
| 1. in the message to be instrumented, change the second argument from DBG_* macro to one of | ||
| - `Critical(id)` | ||
| - `Important(id)` | ||
| - `Dbg(id)` |
There was a problem hiding this comment.
There is no single-parameter Dbg() macro. The existing two-parameter Dbg() macro should not be used where one of the first two macros can be used instead. For PRs that change an existing debugs() message, the correct macro choice is deterministic, and developer documentation, if any, should note that.
Sorry, something went wrong.
| - `Dbg(id)` | ||
| - where the id is the one noted in the first step | ||
| 1. in the file containing that message, it might be necessary to `#include "debug/Messages.h"` | ||
| 1. try a full build |
There was a problem hiding this comment.
Developers should not try a "full build" (whatever that means). They should test that their changes are actually working, at least in their environment! The latter requires some form of a build, of course, but all of those generic requirements should be obvious and should not be documented in every for-developer note like this one. If we want to be explicit, referring to the existing PR preparation checklist may be sufficient.
If you keep the "try a full build" instructions, give a specific command, at least as an example. AFAICT, running rm src/squid; make should be sufficient in this case. Well, to be more precise, make alone should be sufficient, but I am not sure we have fixed all the dependency tracking problems that did require a rm src/squid step in some (fairly typical) cases.
Sorry, something went wrong.
There was a problem hiding this comment.
Adapting with a more generic comment; if a developer is willing to get their hands in the code, they probably know this bit already and I don't want to mansplain
Sorry, something went wrong.
| - where the id is the one noted in the first step | ||
| 1. in the file containing that message, it might be necessary to `#include "debug/Messages.h"` | ||
| 1. try a full build | ||
| 1. submit a PR to the Squid project to share the fruit of the labour :) |
There was a problem hiding this comment.
Please add a link to the PR submission instructions.
Sorry, something went wrong.
| [this is the current list in master](https://github.com/squid-cache/squid/blob/master/doc/debug-messages.dox). | ||
|
|
||
| Message IDs are guaranteed to not change to guarantee forward compatibility. | ||
|
|
There was a problem hiding this comment.
I am strongly against (manually and poorly) duplicating cache_log_message documentation in this repository.
Sorry, something went wrong.
There was a problem hiding this comment.
removing and rewording
Sorry, something went wrong.
| --- | ||
| categories: Feature | ||
| --- | ||
| # Feature: Cache Log Message individual control |
There was a problem hiding this comment.
I am against using Feature format for developer-focused documentation. This page is not about the (already well-documented) Squid feature. It is about helping developers to enhance Squid in a particular way. This information will never be complete and will get stale, but it may still be useful. We should find a better/new/dedicated location (within this repository) for this kind of development advice, so that it is easier to find, and so that notes can share common disclaimers and caveats.
I (still) recommend adding Squid developer "notes" for that purpose and have even published a few to bootstrap that idea (long before this repository was created). If you like that idea, this PR can create something like docs/dev-notes/cache_log_message.md. We could reuse the old docs/ProgrammingGuide, but I do not recommend that (I can detail my reasoning if somebody thinks that reusing ProgrammingGuide is a great idea).
P.S. I do not recommend using the old Feature approach for new user-focused pages. It has been proven problematic, on several levels. We can agree to revamp that approach to address those known shortcomings (but that discussion is probably outside this PR scope).
Sorry, something went wrong.
| 1. in the file containing that message, it might be necessary to `#include "debug/Messages.h"` | ||
| 1. try a full build | ||
| 1. submit a PR to the Squid project to share the fruit of the labour :) | ||
|
|
There was a problem hiding this comment.
We should give an example. After squid-cache/squid#1481 is merged, we can say something like "For an example, see commit ... but use the src/debug/Messages.h macro that matches the debugs() message you are adjusting".
Sorry, something went wrong.
| - `Important(id)` | ||
| - `Dbg(id)` | ||
| - where the id is the one noted in the first step | ||
| 1. in the file containing that message, it might be necessary to `#include "debug/Messages.h"` |
There was a problem hiding this comment.
We should be more specific to be more helpful and to reduce the number of change requests during review:
| 1. in the file containing that message, it might be necessary to `#include "debug/Messages.h"` | |
| 1. If the modified source file does not already include it, `#include "debug/Messages.h"` |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
As it says on the tin