coding-style: propose documentation recommendations #16400

rockwotj · 2024-01-31T15:51:56Z

In an effort to prioritize documentation, add expectations to the coding guide on what should
be documented.

As an example of existing codebase that I think is fairly well documented is CockroachDB:

They have "package" level documentation (which we'll solve with README):
https://github.com/cockroachdb/cockroach/blob/b876c990bbd40af075dda6ab6b28b392ec6da19f/pkg/gossip/doc.go#L12-L15

If you look through the entry point to that exact package, I think it's pretty solid:
https://github.com/cockroachdb/cockroach/blob/master/pkg/gossip/gossip.go

Other package examples from CockroachDB:

As for our own codebase, I think we have parts that are well documented, and places that could use some love. I believe the largest gap at the moment is some "per library documentation" to give an overview of the lay of the land and high level responsibility of a library. I think another thing that can help reduce the surface area of libraries and make them easier to understand is breaking them. For example, the cluster library does so much that it's hard to imagine writing a good README.md there.

Good examples

redpanda/src/v/raft/state_machine_manager.h

Line 65 in 697d4a1

class state_machine_manager final {

redpanda/src/v/storage/segment_appender.h

Line 52 in 5650f5d

class segment_appender {

redpanda/src/v/bytes/include/bytes/iobuf.h

Line 53 in 23c0ce6

class iobuf {

redpanda/src/v/storage/batch_cache.h

Line 101 in 23c0ce6

class batch_cache {

redpanda/src/v/io/cache.h

Line 357 in 23c0ce6

class cache {

Examples that could use some documentation love

redpanda/src/v/cluster/controller.h

Line 39 in 697d4a1

class controller {

redpanda/src/v/kafka/server/group_stm.h

Line 68 in 23c0ce6

class group_stm {

redpanda/src/v/rpc/connection_cache.h

Line 84 in 23c0ce6

class connection_cache final

redpanda/src/v/cloud_storage/base_manifest.h

Line 40 in 23c0ce6

class base_manifest {

redpanda/src/v/cluster/topics_frontend.h

Line 39 in 23c0ce6

class topics_frontend {

redpanda/src/v/cluster/config_frontend.h

Line 18 in 23c0ce6

class config_frontend final {

Backports Required

Release Notes

none

michael-redpanda

love it

michael-redpanda · 2024-01-31T15:59:28Z

src/v/coding-style.md

+Public header files should go in an `includes` directory within the module they belong to. 
+For example, a module named `foo` should have public header files within `src/v/foo/include/foo`.


+1 for this.

Should the include directory end up in target_include_directories(<target> PUBLIC include) so only a subset of headers are exposed from the module?

Would be nice that only some of the headers are actually exposed, rather than our current methodology of putting src/v in include_directory (

redpanda/src/v/CMakeLists.txt

Lines 1 to 2 in 07ea1bc

include_directories(${CMAKE_CURRENT_SOURCE_DIR})

include_directories(${CMAKE_CURRENT_BINARY_DIR})

)?

Noah already has setup the work for this: c9a2dbc

Eventually we can remove the root include, and yes we're working towards having private vs public headers and helping our build system and dependency graph be cleaner.

dotnwat · 2024-01-31T16:07:34Z

src/v/coding-style.md

@@ -6,12 +6,50 @@ The document covers style and convention not enforced by clang-format.

 Header files have the `.h` extension, source files use the `.cc` extension.

+Public header files should go in an `includes` directory within the module they belong to. 
+For example, a module named `foo` should have public header files within `src/v/foo/include/foo`.


dotnwat · 2024-01-31T16:10:55Z

src/v/coding-style.md

+or other common lifecycle methods like `start` and `stop` could be considered trivial. It's highly
+recommended to error on the side of verbosity when documenting functions however.


I wonder if we can find (1) some article about how to write good function documentation and (2) a code base with good examples. Sometimes I catch myself writing prose that matches 1-1 with the code, but I suspect that isn't really the type of doc string we're looking for.

Good call. Let me look around a bit and get back to this

I think cockroach is a decent example.

They have "package" level documentation (which we'll solve with README): https://github.com/cockroachdb/cockroach/blob/master/pkg/gossip/doc.go

If you look through the entry point to that exact package, I think it's pretty solid:
https://github.com/cockroachdb/cockroach/blob/master/pkg/gossip/gossip.go

Another package example from cockroach:
https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/doc.go, or https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sqlstats/ssprovider.go

Anyways, I don't think more text is better, but something is better than nothing. Maybe it's better to surface stuff in our codebase?

Example of something good IMO:

redpanda/src/v/raft/state_machine_manager.h

Line 65 in 697d4a1

class state_machine_manager final {

Example of something that could be better IMO:

redpanda/src/v/cluster/controller.h

Line 39 in 697d4a1

class controller {

This looks great to me!

mmaslankaprv · 2024-02-06T11:20:50Z

this is great

src/v/coding-style.md

travisdowns · 2024-02-06T13:59:05Z

src/v/coding-style.md

@@ -6,12 +6,50 @@ The document covers style and convention not enforced by clang-format.

 Header files have the `.h` extension, source files use the `.cc` extension.

+Public header files should go in an `includes` directory within the module they belong to. 


This may need some clarification of what "public" means here. Is "public" is entirely concerned with whether other redpanda modules #include them directly?

Yes. Will clarify with:

Public header files (files that are allowed to be #included by other modules).

travisdowns · 2024-02-06T14:00:55Z

src/v/coding-style.md

+
+### Modules
+
+Modules themselves should have a `README.md` file explaining the purpose of the module,


What is a module here? A module is a top-level directory in src/v?

Any CMake library target that isn't a test - generally this is one to one with a directory, but increasingly these are not all top-level directories.

Open to better naming because of the confusion with C++ modules.

Open to better naming because of the confusion with C++ modules.

I think module is fine but we should probably have one sentence briefly introducing what a module is, to us, before the point we start using that term.

src/v/coding-style.md

travisdowns · 2024-02-06T14:05:28Z

src/v/coding-style.md

+* Which cores it exists on (if a sharded service)
+* If it is a cluster/node/shard view of information
+
+While only required in public headers, it's also encouraged to add this information to key classes 


Personally I think we should document all classes. Ultimately I feel the "module boundary" is not a very important one here, inside Redpanda, for the purposes of documentation because in general core is working across modules, i.e., it is not as if the public header documentation is consumed by a different set of people. The same reasoning for documenting "public" headers mostly applies to classes anywhere else?

That's fair - again I was trying to find a minimum subset of agreeable points to start this, but I can circle around with various TLs and get everyone's thoughts.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rockwotj · 2024-02-06T15:19:03Z

Updates: Took a stronger stance on documentation and added examples to the cover letter.

dotnwat · 2024-02-06T16:48:48Z

@travisdowns's updates in storage segment appender have really good comments IIRC

vbotbuildovich · 2024-02-06T17:07:46Z

new failures in https://buildkite.com/redpanda/redpanda/builds/44756#018d7f28-ed4c-42a6-9d99-875ff07e5248:

"rptest.tests.recovery_mode_test.RecoveryModeTest.test_recovery_mode"

vbotbuildovich · 2024-02-06T18:16:24Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44756#018d7f51-b23b-4700-ae5b-d786f20dfc30

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44770#018d8020-bd2f-4119-bc71-85e7704ed492

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

The libraries list was out of date, update it to the latest and greatest. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rockwotj · 2024-02-06T18:56:52Z

Updates:

Stronger requirements for documenting structs
Add examples from our code base to cover letter
update subsystem list and descriptions
Add docs guidelines and a couple of posts that might be helpful on writing comments

travisdowns

Looks great.

nvartolomei · 2024-02-08T11:15:13Z

src/v/coding-style.md

+
+For the curious, here are some useful articles on this subject:
+
+* [Coding Horror - Code tells you how, comments tell you why](https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/)


nvartolomei · 2024-02-08T11:18:37Z

src/v/coding-style.md

+### Functions
+
+Non trivial free and class functions should be documented. Trivial is ultimately
+a judgement call, but a rule of thumb is that getter methods (like `iobuf value() const { return _value; }`)


Ideally these would have internally enforced invariants. However, sometimes even a trivial method like that needs documentation about preconditions. E.g. that start() must have been called before.

An assert inside might serve as documentation too. If the method declaration is split from definition then I'd vote for stating the invariants in the declaration (in header file) too.

Totally agree. Happy to take a look at a PR if you want to update this section. I'm not discouraging comments here, but trying to prevent someone feeling burdened to write a useless comment like:

/** * Get the value. */ iobuf value() const { return _value; }

github-actions bot added the area/redpanda label Jan 31, 2024

michael-redpanda previously approved these changes Jan 31, 2024

View reviewed changes

michael-redpanda assigned rockwotj Jan 31, 2024

rockwotj requested review from dotnwat, travisdowns and mmaslankaprv January 31, 2024 16:06

dotnwat previously approved these changes Jan 31, 2024

View reviewed changes

mmaslankaprv previously approved these changes Feb 6, 2024

View reviewed changes