14 ms·
Uber Go Style Guide
- ra7 7y agoIs Go the primary language in Uber now? I see a lot of tools written in Go coming out of Uber. What kind of services inside Uber is Go used for?
- ryanworl 7y agoUber's distributed time series database M3 (https://m3db.io https://m3db.io) is written in Go.
- yibilabi 7y agoYes, Go is the primary language in Uber.
- ladysman217 7y agoGo is pretty good
- xyzzy_plugh 7y agoFor backends they've standardized on Go and Java, deprecating/eliminating node and python where possible. I understand node is still prevalent for serving react-based frontends, both external and internal facing.
- dev_dull 7y agoHow do they decide with a new backend service to use Go vs Java?
- benlc 7y agoOften comes down to the preference of the engineers in a given team and evaluating pros and cons with engineers outside their team (aka review their plans before they start building)
- xyzzy_plugh 7y agoI'd guess it's usually up to the developers/owning team, but also to some extent depending on the ecosystem it's interacting with (e.g. Hadoop would tend to prefer Java)
- badrequest 7y agoUber have well over 1,500 microservices written in Go, it is the primary language backend services are written in at Uber.
- OJFord 7y ago> Uber have well over 1,500 microservices written in Go 1500?! I can't even imagine how micro a microservice could be such that Uber could have so many. edit: Having said that, the Monzo blog post also on the front page says it has 1100 microservices, so I suppose I've just not worked somewhere with 'actual' microservices. What constitutes a 'service', a single procedure for RPC?
- pjmlp 7y agoI guess they suffer from "leftpad as service". http://left-pad.io/ http://left-pad.io/
- sidlls 7y agoThe target is one microservice per syscall, maybe.
- deleted 7y ago[deleted]
- badrequest 7y agoI don't work at Uber, so I can't answer your questions around motivation, but I took the 1,500 value from this presentation at Gophercon this year: https://www.youtube.com/watch?v=nLskCRJOdxM&t=22m59s https://www.youtube.com/watch?v=nLskCRJOdxM&t=22m59s
- jpalomaki 7y agoI would imagine quite a services to just deal with legislation/taxation stuff in all the different markets they operate in. These can also result on several integrations to government systems - all country specific. In some markets they are partnering with taxi companies - maybe more integrations for ordering and reporting. In general it is easy to underestimate the complexity of systems you are not familiar with. The devil is in the details and deeper you look, more complicated it usually gets.
- kerng 7y agoProbabaly more of an emotional, rather then rational decision. I heard that many Go folks from Google joined Uber. Looking at the stock, they probably should focus on building new things/better the business model, rather then rewriting things in Go.
- mytailorisrich 7y agoWhat else have they got to do, though? Uber have thousands of engineers to develop and maintain a taxi hailing app...
- ninkendo 7y agoWork on their internal chat app, I guess? https://eng.uber.com/uchat/ https://eng.uber.com/uchat/
- xmprt 7y agoThat's built on top of Mattermost which is open source. They do build their own infra instead of using cloud though so that probably uses a big part of it.
- mytailorisrich 7y agoHowever you look at it, Uber and other new so-called tech companies these days spend like there is no tomorrow instead of focusing on the bottom line.
- weberc2 7y agoI'll bite. Why do you suppose it's an emotional decision? And why do you assume they're rewriting things and not writing new code in support of their business model?
- kerng 7y agoBecause they have publicly talked about it: https://eng.uber.com/schemaless-rewrite/ https://eng.uber.com/schemaless-rewrite/
- sanxiyn 7y ago"Copy Slices and Maps at Boundaries. Slices and maps contain pointers to the underlying data so be wary of scenarios when they need to be copied. Keep in mind that users can modify a map or slice you received as an argument if you store a reference to it. Similarly, be wary of user modifications to maps or slices exposing internal state." This could be used as an ad for Rust borrow checker, verbatim. You can't modify a map or slice you passed as an argument if a reference to it is stored!
- apta 7y agoAnother solution is to use immutable and/or persistent data structures. Of course, because golang doesn't have generics, it becomes unwieldy to have a library of them, unlike what we see in Java, Scala, etc. where these enjoy a wider adoption.
- sanxiyn 7y agoShared mutable state is evil, so "not mutable" has been proposed as a solution. Rust is different, because Rust's solution is "not sharing". You can still mutate!
- reificator 7y ago> Rust is different, because Rust's solution is "not sharing". You can still mutate! For the record, rust does also default to immutability, and you can share immutable references all you'd like.
- bcrosby95 7y agoI would describe Rust more like a read-write lock than not sharing.
- nine_k 7y agoA lock is acquired and detected at runtime. Ownership and move semantics are checked at compile time, and allow effective "not sharing" without waiting or defensive copies.
- thallavajhula 7y agoCorrect me if I'm wrong, but, isn't the primary purpose of `gofmt` to solve having to deal with style guides like these in the community?
- mattcdrake 7y agoThis is referenced in the first paragraph of the style guide: "Styles are the conventions that govern our code. The term style is a bit of a misnomer, since these conventions cover far more than just source file formatting—gofmt handles that for us."
- knorker 7y agoPretty good. Some opinions that fall under "be consistent within our code base" (which is why Uber should have a style guide at all), so all good. Just one comment though: > Embedding sync.Mutex Never do this on an exported type. If you embed sync.Mutex that makes "Lock" and "Unlock" part of your exported interface. Now the caller of your API doesn't know if they are supposed to call Lock/Unlock, or if this is a pure internal implementation detail. your `type Stats` "isn't" a mutex. It has a mutex.
- meddlepal 7y ago> Never do this on an exported type. If you embed sync.Mutex that makes "Lock" and "Unlock" part of your exported interface. Now the caller of your API doesn't know if they are supposed to call Lock/Unlock, or if this is a pure internal implementation detail. Isn't that exactly what they say? > Embed for private types or types that need to implement the Mutex interface. Personally, I would never embed a sync.Mutex though because even for an internal type it is confusing.
- knorker 7y agoCurious. Under "Returning Slices and Maps" they use it in their "Good" example.
- ArtWomb 7y agoAgree, but I can't recall ever seeing a package in the wild that labels itself as "concurrency-safe" and yet requires the caller to explicitly call Lock! HashMap is probably a good example https://github.com/cornelk/hashmap https://github.com/cornelk/hashmap
- logicallee 7y agoNot an expert, but could someone explain why it says: "Panic/recover is not an error handling strategy. A program must panic only when something irrecoverable happens such as a nil dereference." Why is that any more irrecoverable than anything else? (You can check if it's nil before referencing it, right?)
- ArtWomb 7y agoNo buffered channels, or if you do, you must provide a very strong rationale ;) I still use them quite a bit. Particularly synchronizing large rule sets as bit arrays. Of course you can use sync.Wait instead. But make(chan bool, N) semantics are just more convenient. It's stealth synchronization as a by-product. And hence the warnings about determinism!
- dev_dull 7y agoI'd like to hear more about this rational. With a single writer, buffering channels can help smooth out inputs when the p90 is much higher than the average with channel writers but not readers. At least that's my impression.
- jerf 7y agoPut in English, the type of an unbuffered channel is "a channel that always blocks on writing until the value has been read". The type of a buffered channel is "a channel that doesn't block when written to, until it is full in which case it blocks on writing until some other value has been read by some other unrelated goroutine". The former is a reasonable concurrency primitive. The latter superficially seems similar, but is actually a much more complicated primitive, with the corresponding code understanding problems and the increased likelihood of more concurrency problems being hidden at low scale but coming out at scale (mostly deadlocks), plus the fact it seems similar is also problematic. The fact that the Go type system does not allow distinguishing between the two is also problematic. It has some special-case purposes, but I also always scrutinize any channel created with buffering to make sure it is one of those special cases. Buffered channels have not impressed me with their ability to do any sort of performance improvements. In almost all cases, if you've filled up your readers, you really want your writer to block. It provides cheap, effective backpressure; arguably for software-at-scale it's the most useful aspect of the channel primitive.
- boyter 7y agoBackpressure to writers is the main reason to use buffer channels IMHO. I have never seen any performance differences between buffered or unbuffered for anything I have used. The other reason to use them is controlling CPU. Spin up a pool of consumers and control how many cores they use with the buffer.
- dev_dull 7y agoA style guide with benchmarks that can be read in a single cup of coffee. Very nice and one of the reasons I enjoy Go!
- sidlls 7y agoThe section on exporting functions to check errors just exposes Go's weakness in error checking. It's unnecessarily verbose.
- badrequest 7y agoThe way to do this properly got much better in the most recent Go release, the linked documentation doesn't yet take that into account, it would seem. https://github.com/golang/go/wiki/ErrorValueFAQ https://github.com/golang/go/wiki/ErrorValueFAQ
- sidlls 7y agoError handling in go would be much better with a proper sum type, though. It's very clunky as-is, even with the improvements outlined in that link
- thinkingkong 7y agoHow would you enforce any of these? Are these guides or reasons to not accept PRs? Seems like it would be worth encoding in a tool if this is for teams.
- mav3rick 7y agoMostly by people following these and you bootstrap by some trusted people getting +2 rights. As more people are trusted they get the rights.
- codesushi42 7y agoWhy is this better than Google's?
- jrockway 7y agoThis, to me, seems largely a superset of Google's style guide. There is some specific guidance for working with channels, mutexes, and atomics. My impression from my time at Google was that the Go team really expects you to never use mutexes and atomics, preferring goroutines for all synchronization. Sometimes that is impractical, though.
- nergal 7y agoUnderscore in globals. Is that really go best practice?
- tomohawk 7y agoYes. Variables that start with lower case look like a locally defined variable in a func. Adding the underbar makes it absolutely clear, and prevents shadowing, or the need to work around shadowing with a slightly different name.
- yencabulator 7y agoI've never heard or seen that, outside of this document. So I would say no, it's not a Go practice, it's an Uber practice.
- heroHACK17 7y agoI really like the horizontal 'Good/Bad' code comparisons in this guide. I didn't realize how horizontal vs. vertical code comparison affects readability; IMO horizontal is MUCH more readable. Example: https://github.com/uber-go/guide/blob/master/style.md#defer-to-clean-up https://github.com/uber-go/guide/blob/master/style.md#defer-...
- kerng 7y agoAgreed. This makes it much better parsable! Although on the phone I initially didn't realize there was horizontal content/scrolling! Thanks for pointing that out! Very neat.
- klabb3 7y ago+1. It is quite an achievement by Go as a language and/or community to have generally short lines of code where two columns fit in a regular website width. I don't like everything about Go, but it's usually pretty easy on the eyes for this reason.
- apta 7y agoI don't think that the prevalence of single letter variable names can be considered an achievement (which is how they get short lines to begin with). It's quite a hindrance to readability. golang as a language does nothing to support shorter lines.
- morelisp 7y ago> golang as a language does nothing to support shorter lines. It does a few things. - Most declarations, including method declarations, take place at the top level. This is unlike most languages which declare methods one level indented along with the object's fields. - Short type definitions (`type T1 T2`) are encouraged and often used for any repeated non-scalar type. This shortens argument specifications and (if a good name was chosen) improves readability. - Anonymous struct fields are shorter both to declare and use. In class-based OO this is often the case for subclasses but not wrappers/facades. Go's approach covers both. - Go linters push people to avoid `if x { ... return a } else { ... return b }` in favor of `if x { ... return a } ... return b`. I actually hate this, but it does keep indentation down. - The lack of exception handling means there is a constant "pressure" to push errors back up manually, via early return if it's locally unhandled or to the top of the current lexical scope of it is. The result is definitely verbose and one of the easiest parts of Go to criticize - but the same pressure results in short (but plentiful) error handling lines. (I do agree the majority of savings comes from short variable names, though I disagree that it affects readability much for the case of _variable names_ - struct fields are another issue...)
- jrockway 7y agoSomething that I don't see style guides addressing, but think they should, is how to cancel work when the caller no longer cares about the answer. (Consider an aborted RPC, or someone pressing the "stop" button in their browser.) A lot of people will do things like: func (s *Server) HandleFoo(ctx context.Context, in *input) status { ch <- workFor(in) return status.Accepted } This will block on the channel write even if the context becomes cancelled. Instead, you really need to be explicit about what you want to block: func (s *Server) HandleFoo(ctx context.Context, in *input) status { select { case ch <- workFor(in): return status.Accepted case <- ctx.Done(): return status.Error("handle foo: wait for work to be accepted: %w", ctx.Err()) } } Now we don't wait for the work to be accepted if the caller decides it doesn't care about the job anymore. My impression from digging around the open source world is that nobody except me cares about this. So maybe I'm just crazy. But I really like not leaking goroutines on long-running programs, making Control-C behave gracefully, etc.
- deleted 7y ago[deleted]
- thezviad 7y agothe way to do is to pass context all the way through, until the other thing you are waiting on uses up no resources. so in your example it should be done like this: ch <- workFor(ctx, in) and the 'workFor' function should be the one that gets canceled with the context. Deep down at the very end, you would have select statement with '<-ctx.Done()' and something else that doesn't need canceling (if it needs canceling it should have taken context too). Of course the world isn't perfect and not everything takes context right now (even in standard library), so you do have to do some workarounds every once in a while.
- jrockway 7y agoThis is a very trivial example so doesn't dive into all the complexity. There is a lot of nuance here, like whether or not you really want to do an operation in the background in the first place. Background work does means that you lose the ability to apply backpressure to the calling system, and that will cause a lot of problems under load. Even if you do want to do the operation in the background, you still want to provide a (derived) context so that you can link the background work to the initial request (at the very least, for tracing purposes). That is omitted here for simplicity. Where I was going with all of this was that the linked style guide mentions cases where people create buffered channels presumably because their channel writes start blocking. No buffer will make up for the case where the rate of work requested is higher than the rate at which work can be completed. What people really want in that case is the ability to get out of the channel write and return an error to the downstream system; you don't want to buffer, you want to cancel. There are many ways to accomplish the act of cancelling work, but you do have to watch your channel writes explicitly.
- abbot2 7y agoI find it amusing that the advice in the style guide gives a good example contradicting another good example, and contains a subtle bug. In the "Reduce Scope of Variables", second good example leaks an open file when WriteString fails, because it doesn't follow the own advice of "Defer to Clean Up" if you are curious. (Handling that properly with a defer is a bit more tricky - something like https://play.golang.org/p/l1PeWM3Tisg https://play.golang.org/p/l1PeWM3Tisg). Update: style guide was fixed after this report :) It was this if you wonder: https://github.com/uber-go/guide/blob/a53ee0bef8c0b11b52340dbbc82e8be37f4a88d2/style.md https://github.com/uber-go/guide/blob/a53ee0bef8c0b11b52340d...
- fryh 7y agoI am not a Go programmer, but this seems like a really nice guide. Is there anything similar for Python or C++?
- magicbuzz 7y agoYes, it's great. I know a few folks working on Go backends - I'm going to point this out to them. And I think like you, it raises the question of 'Is there a guide like this for what I'm working in?' In my early React years, I had a lot of conversations about good vs bad as we got used to working in a declarative component hierarchy rather than the old imperative (tweak that DOM!) way. I still come across projects where people write html + bootstrap translated into a render method rather than using the power that is components.
- tapirl 7y agoCopy Slices and Maps at Boundaries: https://github.com/uber-go/guide/blob/master/style.md#copy-slices-and-maps-at-boundaries https://github.com/uber-go/guide/blob/master/style.md#copy-s... The suggested good clone is not very efficient: https://github.com/go101/go101/wiki/How-to-efficiently-clone-a-slice%3F https://github.com/go101/go101/wiki/How-to-efficiently-clone... In fact, I prefer the suggested bad one instead. We should let the caller to determine whether or not a clone is needed. -------------------------- Start Enums at One: https://github.com/uber-go/guide/blob/master/style.md#start-enums-at-one https://github.com/uber-go/guide/blob/master/style.md#start-... Any reason here? (Edit: I just missed the reason. However, I think there should be a default value for most enums and most of the default values should zero.) -------------------------- Local Variable Declarations: https://github.com/uber-go/guide/blob/master/style.md#local-variable-declarations https://github.com/uber-go/guide/blob/master/style.md#local-... Personally, I prefer "var x = ..." and think short variables should be used as limited as possible. I remember that Go team has a not-very-concrete plan to depreciate short variable declarations. -------------------------- Avoid Naked Parameters: https://github.com/uber-go/guide/blob/master/style.md#avoid-naked-parameters https://github.com/uber-go/guide/blob/master/style.md#avoid-... This suggestion has its own drawback. When a parameter name changed, all the places using it must be modified to keep consistent. This is one reason why Go team rejected named arguments. If this must be done, I recommend to define and use some named bool constants instead.
- sanxiyn 7y agoRe: Start Enums at One. As explained, it is to avoid making the first enum value the default when making it the default is not appropriate.
- gen220 7y agoAt my place of work, we pretty much always make the default (0) value “Unspecified”. The rationale is, it’s often quite useful to know that the originator hasn’t specified a value, and to help new originators avoid unintended side-effects. This style guide has the same effect, but it’s implicit; takes a few more mental cycles to parse in exchange for less code. Personally, I think it’s probably worth the trade off to type it out once and save time thinking later.
- palebluedot 7y agoThe preference of channel size being unbuffered or just 1 is interesting. That seems like something specific to a problem domain; for instance, in projects I am working on now, having a large buffered channel (1000s deep) is useful for worker queues of thousands of goroutines, that all read from a task feeder channel. This type of queuing seems go-idiomatic, and negates the need for additional synchronization. In this case, the backpressure blocking on writers is a feature.
- baby 7y agoMaybe to avoid deadlocks?
- fishywang 7y agoWhy having a 1000s buffered channel is useful in this case? If it's unbuffered, you still get the backpressure blocking on writers as a feature, since you have a worker queues of thousands of goroutines to read and handle them.
- palebluedot 7y agoThat's a good point, at steady-state, I'd imagine unbuffered channels would have the same throughput as a deeply buffered channels. The main advantage is being able to spool up faster and smooth out throughput, but it could be for most workloads that is not valuable enough. Perhaps I placed too much value on that. Your comment (and others) have convinced me to do some more empirical testing and see how necessary buffered channels are for my goal.
- dallbee 7y agoA lot of my coworkers were getting hung up on this point too. I think they're just emphasizing that large channel buffers can hide concurrency problems, so you want to be careful when using them. The last sentence in the recommendation emphasizes this: use them with scrutiny.
- Matthias247 7y agoPicking a queue size is a latency/throughput/determinism tradeoff. Picking a higher queue size can increase the peak throughput. The queue will smoothen out peaks and valleys in the workload, and the other end of the queue will get rarer into situations where there is no work. If you know the duration of the "valleys" in your workload you can size your queue exactly to get over them. However a too big queue size can easily lead to higher latencies. In extreme situations the work might be already outdated at the point of time it's processed by the receiver. And backpressure on the producer is less given. Queue sizes > 1 can also mask concurrency issues (like deadlocks), which will then only show up rarely in production when the queue is fully exhausted. I guess that's one of the main reasons why they picked the 0/1 rule.
- marcrosoft 7y agoIt’s nice that go barely needs a style guide because of go fmt.
- jlokier 7y agoThis recommendation surprised me: if err := ioutil.WriteFile(name, data, 0644); err != nil { return err } I've often seen guides for many languages that say don't use an assignment as an "if" condition. It may be a typo, and so is a source of errors, or it hides errors. Many compilers warn about it, and some people will consider it poor enough to refactor it out. Of course it's not the assignment which is being tested in the Go code above. But it might as well be. In Swift, Rust, Clojure and Perl, "if let" is common. ("if-let" in Clojure, "if my" in Perl). It's useful, and conforms well to the idea of limiting scope of the tested variable. I use it all the time. So scope limiting in "if" is a good idea. But in those languages, they have the benefit of a clear syntax with a keyword, and it's a single assignment+test operator, very unlikely to be an accident due to a typo, or to hide an accident. In contrast, I think the Go snippet looks error prone because the actual condition is all the way off the right. The assignment is obvious and the intention to test it can be presumed when skimming the code. But because the test is way off the right, at a horizontal position which will vary in different code, and at the end of a long line with a compound statement, I think it will be easy when skimming code to fail to notice if the condition is wrong due to a typo. So I'm surprised Uber recommends this one.
- PudgePacket 7y agoIt helps reduce uninitialised variables, and if the condition is false, you don't have an extra variable hanging around in the scope that could potentially be misused by the developer later.
- jlokier 7y agoYou seem to have missed the point of the comment you're replying to, which already acknowledges that scope-limiting is a good thing to do.
- Groxx 7y agoI'll throw in one request for functional options as written here: please don't use closures to capture the data. If you use closures, it's nearly impossible to compare the results for equality in tests or code that might care to validate / deduplicate / whatever those args. You can achieve it with a moderate amount of heavily-implementation-dependent reflection (get the func type, construct arg(s), call func, check result), but that's about it. Please just use values as the backing type. `type thing int` is utterly trivial to compare if needed, by comparison - just use `==`. Any code anywhere can construct the same argument in the same way and `==` to see if what they are checking is part of the args, and you're still not binding yourself to a specific implementation-type that'll later risk a compile-time failure. (if users are casting to the private `int` type to check stuff, yea, it breaks - but the reflection-to-read-closure approach has that same problem, you can't stop it) Also, in my experience, the "loop and apply" pattern tends to fall apart rather quickly, and you start wanting more complicated interactions between args / validation that you didn't pass both "include deleted" and "exclude deleted" and the second just silently clobbered the first. With values you can pretty easily loop and switch on the type and do whatever you need.
- jonahx 7y agoare you saying you’re against the functional options approach? can you provide an example of a testing difficulty you’ve had?
- Groxx 7y agoI mean that this: func MyOption(arg int) Option { return myoption(func(opts type) { opts.thing = arg }) } will result in this: MyOption(5) == MyOption(5) // false Which makes it a pain in 1) testing, since you can't see the options that were passed, and 2) middleware, since you can't see the options that were passed, to validate or extend them safely. And 3) debuggers or Printf since you can't see the value in the closure until it's executed. Instead do: type myOption int func MyOption(arg int) Option { return myOption(arg) } since it has none of those problems. Whether you use the "loop and opt.apply(arg)" pattern or not doesn't really matter - that's a purely internal detail (personally I find it over-simplifies things and causes problems). Just "avoid passing values through func closures".
- aloknnikhil 7y ago> Use go.uber.org/atomic Atomic operations with the sync/atomic package operate on the raw types (int32, int64, etc.) so it is easy to forget to use the atomic operation to read or modify the variables. go.uber.org/atomic adds type safety to these operations by hiding the underlying type. I don't agree with this guideline. When you anyway have to remember to invoke an intrinsic function on the variable (unlike, say, operator overloading), I wonder why it's necessary to include yet another dependency that's just a wrapper for sync/atomic all because the developer doesn't pay attention to the right use of atomics.