1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
|
# Gitaly code style
## Character set
### Avoid non-ASCII characters in developer-facing code
Code that is developer-facing only, like variables, functions or test
descriptions should use the ASCII character set only. This is to ensure that
code is accessible to different developers with varying setups.
## Errors
### Use %v when wrapping errors
Use `%v` when wrapping errors with context.
fmt.Errorf("foo context: %v", err)
### Keep errors short
It is customary in Go to pass errors up the call stack and decorate
them. To be a good neighbor to the rest of the call stack we should keep
our errors short.
// Good
fmt.Errorf("peek diff line: %v", err)
// Too long
fmt.Errorf("ParseDiffOutput: Unexpected error while peeking: %v", err)
### Use lower case in errors
Use lower case in errors; it is OK to preserve upper case in names.
### Errors should stick to the facts
It is tempting to write errors that explain the problem that occurred.
This can be appropriate in some end-user facing situations, but it is
never appropriate for internal error messages. When your
interpretation is wrong it puts the reader on the wrong track.
Stick to the facts. Often it is enough to just describe in a few words
what we were trying to do.
### Use %q when interpolating strings
Unless it would lead to incorrect results, always use `%q` when
interpolating strings. The `%q` operator quotes strings and escapes
spaces and non-printable characters. This can save a lot of debugging
time.
## Logging
### Use context-based logging
The `ctxlogrus` package allows to extract a logger from the current
`context.Context` structure. This should be the default logging facility, as it
may carry additional context-sensitive information like the `correlation_id`
that makes it easy to correlate a log entry with other entries of the same
event.
### Errors
When logging an error, use the `WithError(err)` method.
### Use the `logrus.FieldLogger` interface
In case you want to pass around the logger, use the `logrus.FieldLogger`
interface instead of either `*logrus.Entry` or `*logrus.Logger`.
### Use snake case for fields
When writing log entries, you should use `logger.WithFields()` to add relevant
metadata relevant to the entry. The keys should use snake case:
```golang
logger.WithField("correlation_id", 12345).Info("StartTransaction")
```
### Use RPC name as log message
In case you do not want to write a specific log message, but only want to notify
about a certain function or RPC being called, you should use the function's name
as the log message:
```golang
func StartTransaction(id uint64) {
logger.WithField("transaction_id", id).Debug("StartTransaction")
}
```
### Embed package into log entries
In order to associate log entries with a given code package, you should add a
`component` field to the log entry. If the log entry is generated in a method,
the component should be `$PACKAGE_NAME.$STRUCT_NAME`:
```golang
package transaction
type Manager struct {}
func (m Manager) StartTransaction(ctx context.Context) {
ctxlogrus.Extract(ctx).WithFields(logrus.Fields{
"component": "transaction.Manager",
}).Debug("StartTransaction")
}
```
## Literals and constructors
### Use "address of struct" instead of new
The following are equivalent in Go:
```golang
// Preferred
foo := &Foo{}
// Don't use
foo := new(Foo)
```
There is no strong reason to prefer one over the other. But mixing
them is unnecessary. We prefer the first style.
### Use hexadecimal byte literals in strings
Sometimes you want to use a byte literal in a Go string. Use a
hexadecimal literal in that case. Unless you have a good reason to use
octal of course.
```golang
// Preferred
foo := "bar\x00baz"
// Don't use octal
foo := "bar\000baz"
```
Octal has the bad property that to represent high bytes, you need 3
digits, but then you may not use a `4` as the first digit. 0377 equals
255 which is a valid byte value. 0400 equals 256 which is not. With
hexadecimal you cannot make this mistake because the largest two digit
hex number is 0xff which equals 255.
## Functions
### Method Receivers
Without any good reason, methods should always use value receivers, where good
reasons include (but are not limited to) performance/memory concerns or
modification of state in the receiver. Otherwise, if any of the type's methods
requires a pointer receiver, all methods should be pointer receivers.
### Don't use "naked return"
In a function with named return variables it is valid to have a plain
("naked") `return` statement, which will return the named return
variables.
In Gitaly we don't use this feature. If the function returns one or
more values, then always pass them to `return`.
## Ordering
### Declare types before their first use
A type should be declared before its first use.
## Tests
### Table-driven tests
We like table-driven tests ([Table-driven tests using subtests](https://blog.golang.org/subtests#TOC_4.), [Cheney blog post], [Golang wiki]).
- Use [subtests](https://blog.golang.org/subtests#TOC_4.) with your table-driven tests, using `t.Run`:
```
func TestTime(t *testing.T) {
testCases := []struct {
gmt string
loc string
want string
}{
{"12:31", "Europe/Zuri", "13:31"},
{"12:31", "America/New_York", "7:31"},
{"08:08", "Australia/Sydney", "18:08"},
}
for _, tc := range testCases {
t.Run(fmt.Sprintf("%s in %s", tc.gmt, tc.loc), func(t *testing.T) {
loc, err := time.LoadLocation(tc.loc)
if err != nil {
t.Fatal("could not load location")
}
gmt, _ := time.Parse("15:04", tc.gmt)
if got := gmt.In(loc).Format("15:04"); got != tc.want {
t.Errorf("got %s; want %s", got, tc.want)
}
})
}
}
```
[Cheney blog post]: https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go
[Golang wiki]: https://github.com/golang/go/wiki/TableDrivenTests
## Black box and white box testing
The dominant style of testing in Gitaly is "white box" testing, meaning
test functions for package `foo` declare their own package also to be
`package foo`. This gives the test code access to package internals. Go
also provides a mechanism sometimes called "black box" testing where the
test functions are not part of the package under test: you write
`package foo_test` instead. Depending on your point of view, the lack of
access to package internals when using black-box is either a bug or a
feature.
As a team we are currently divided on which style to prefer so we are
going to allow both. In areas of the code where there is a clear
pattern, please stick with the pattern. For example, almost all our
service tests are white box.
## Prometheus metrics
Prometheus is a great tool to collect data about how our code behaves in
production. When adding new Prometheus metrics, please follow the [best
practices](https://prometheus.io/docs/practices/naming/) and be aware of
the
[gotchas](https://prometheus.io/docs/practices/instrumentation/#things-to-watch-out-for).
## Git Commands
Gitaly relies heavily on spawning git subprocesses to perform work. Any git
commands spawned from Go code should use the constructs found in
[`safecmd.go`](internal/git/safecmd.go). These constructs, all beginning with
`Safe`, help prevent certain kinds of flag injection exploits. Proper usage is
important to mitigate these injection risks:
- When toggling an option, prefer a longer flag over a short flag for
readability.
- Desired: `git.Flag{Name: "--long-flag"}` is easier to read and audit
- Undesired: `git.Flag{Name: "-L"}`
- When providing a variable to configure a flag, make sure to include the
variable after an equal sign
- Desired: `[]git.Flag{Name: "-a="+foo}` prevents flag injection
- Undesired: `[]git.Flag(Name: "-a"+foo)` allows flag injection
- Always define a flag's name via a constant, never use a variable:
- Desired: `[]git.Flag{Name: "-a"}`
- Undesired: `[]git.Flag{Name: foo}` is ambiguous and difficult to audit
## Go Imports Style
When adding new package dependencies to a source code file, keep all standard
library packages in one contiguous import block, and all third party packages
(which includes Gitaly packages) in another contiguous block. This way, the
goimports tool will deterministically sort the packages which reduces the noise
in reviews.
Example of **valid** usage:
```go
import (
"context"
"io"
"os/exec"
"gitlab.com/gitlab-org/gitaly/internal/command"
"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
"gitlab.com/gitlab-org/gitaly/internal/git/repository"
)
```
Example of **invalid** usage:
```go
import (
"io"
"os/exec"
"context"
"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
"gitlab.com/gitlab-org/gitaly/internal/git/repository"
"gitlab.com/gitlab-org/gitaly/internal/command"
)
```
## Goroutine Guidelines
Gitaly is a long lived process. This means that every goroutine spawned carries
liability until either the goroutine ends or the program exits. Some goroutines
are expected to run until program termination (e.g. server listeners and file
walkers). However, the vast majority of goroutines spawned are in response to
an RPC, and in most cases should end before the RPC returns. Proper cleanup of
goroutines is crucial to prevent leaks. When in doubt, you can consult the
following guide:
### Is A Goroutine Necessary?
Avoid using goroutines if the job at hand can be done just as easily and just as well without them.
### Background Task Goroutines
These are goroutines we expect to run the entire life of the process. If they
crash, we expect them to be restarted. If they restart often, we may want a way
to delay subsequent restarts to prevent resource consumption. See
[`dontpanic.GoForever`] for a useful function to handle goroutine restarts with
Sentry observability.
### RPC Goroutines
These are goroutines created to help handle an RPC. A goroutine that is started
during an RPC will also need to end when the RPC completes. This quality makes
it easy to reason about goroutine cleanup.
#### Defer-based Cleanup
One of the safest ways to clean up goroutines (as well as other resources) is
via deferred statements. For example:
```go
func (scs SuperCoolService) MyAwesomeRPC(ctx context.Context, r Request) error {
done := make(chan struct{}) // signals the goroutine is done
defer func() { <-done }() // wait until the goroutine is done
go func() {
defer close(done) // signal when the goroutine returns
doWork(r)
}()
return nil
}
```
Note the heavy usage of defer statements. Using defer statements means that
clean up will occur even if a panic bubbles up the call stack (**IMPORTANT**).
Also, the resource cleanup will
occur in a predictable manner since each defer statement is pushed onto a LIFO
stack of defers. Once the function ends, they are popped off one by one.
### Goroutine Panic Risks
Additionally, every new goroutine has the potential to crash the process. Any
unrecovered panic can cause the entire process to crash and take out any in-
flight requests (**VERY BAD**). When writing code that creates a goroutine,
consider the following question: How confident are you that the code in the
goroutine won't panic? If you can't answer confidently, you may want to use a
helper function to handle panic recovery: [`dontpanic.Go`].
### Limiting Goroutines
When spawning goroutines, you should always be aware of how many goroutines you
will be creating. While cheap, goroutines are not free. Consult the following
questions if you need help deciding if goroutines are being improperly used:
1. How many goroutines will it take the task/RPC to complete?
- Fixed number - 👍 Good
- Variable number - 👇 See next question...
1. Does the goroutine count scale with a configuration value (e.g. storage
locations or concurrency limit)?
- Yes - 👍 Good
- No - 🚩 this is a red flag! An RPC where the goroutines do not scale
predictably will open up the service to denial of service attacks.
[`dontpanic.GoForever`]: https://pkg.go.dev/gitlab.com/gitlab-org/gitaly/internal/dontpanic?tab=doc#GoForever
[`dontpanic.Go`]: https://pkg.go.dev/gitlab.com/gitlab-org/gitaly/internal/dontpanic?tab=doc#Go
|