From 0e7a495829f99edf0cb2bc4288b5059cdb6099b5 Mon Sep 17 00:00:00 2001 From: Emil Tullstedt Date: Thu, 30 Jun 2022 16:47:10 +0200 Subject: [PATCH] Docs: Update backend architecture contributor documentation (#51172) Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> --- .../architecture/backend/communication.md | 179 +++++++------- contribute/architecture/backend/errors.md | 81 +++++++ .../architecture/backend/package-hierarchy.md | 221 +++++++++++++++++- 3 files changed, 380 insertions(+), 101 deletions(-) create mode 100644 contribute/architecture/backend/errors.md diff --git a/contribute/architecture/backend/communication.md b/contribute/architecture/backend/communication.md index f59f598cdf5..151da5978fc 100644 --- a/contribute/architecture/backend/communication.md +++ b/contribute/architecture/backend/communication.md @@ -2,9 +2,95 @@ Grafana uses a _bus_ to pass messages between different parts of the application. All communication over the bus happens synchronously. -> **Deprecated:** The bus has officially been deprecated, however, we're still using the command/query objects paradigms. +## Commands and queries -There are three types of messages: _events_, _commands_, and _queries_. +Grafana structures arguments to [services](services.md) using a command/query +separation where commands are instructions for a mutation and queries retrieve +records from a service. + +Services should define their methods as `func[T, U any](ctx context.Context, args T) (U, error)`. + +Each function should take two arguments. First, a `context.Context` that +carries information about the tracing span, cancellation, and similar +runtime information that might be relevant to the call. Secondly, `T` is +a `struct` defined in the service's root package (see the instructions +for [package hierarchy](package-hierarchy.md)) that contains zero or +more arguments that can be passed to the method. + +The return values is more flexible, and may consist of none, one, or two +values. If there are two values returned, the second value should be +either an `bool` or `error` indicating the success or failure of the +call. The first value `U` carries a value of any exported type that +makes sense for the service. + +Following is an example of an interface providing method signatures for +some calls adhering to these guidelines: + +``` +type Alphabetical interface { + // GetLetter returns either an error or letter. + GetLetter(context.Context, GetLetterQuery) (Letter, error) + // ListCachedLetters cannot fail, and doesn't return an error. + ListCachedLetters(context.Context, ListCachedLettersQuery) Letters + // DeleteLetter doesn't have any return values other than errors, so it + // returns only an error. + DeleteLetter(context.Contxt, DeleteLetterCommand) error +} +``` + +> Because we request an operation to be performed, command are written in imperative mood, such as `CreateFolderCommand`, `GetDashboardQuery` and `DeletePlaylistCommand`. + +The use of complex types for arguments in Go means a few different +things for us, it provides us with the equivalent of named parameters +from other languages, and it reduces the headache of figuring out which +argument is which that often occurs with three or more arguments. + +On the flip-side, it means that all input parameters are optional and +that it is up to the programmer to make sure that the zero value is +useful or at least safe for all fields and that while it's easy to add +another field, if that field must be set for the correct function of the +service that is not detectable at compile time. + +### Queries with Result fields + +Some queries have a Result field that is mutated and populated by the +method being called. This is a remainder from when the _bus_ was used +for sending commands and queries as well as for events. + +All bus commands and queries had to implement the Go type +`func(ctx context.Context, msg interface{}) error` +and mutation of the `msg` variable or returning structured information in +`error` were the two most convenient ways to communicate with the caller. + +All `Result` fields should be refactored so that they are returned from +the query method: + +``` +type GetQuery struct { + Something int + + Result ResultType +} + +func (s *Service) Get(ctx context.Context, cmd *GetQuery) error { + // ...do something + cmd.Result = result + return nil +} +``` + +should become + +``` +type GetQuery struct { + Something int +} + +func (s *Service) Get(ctx context.Context, cmd GetQuery) (ResultType, error) { + // ...do something + return result, nil +} +``` ## Events @@ -44,92 +130,3 @@ if err := s.bus.Publish(event); err != nil { return err } ``` - -## Commands - -A command is a request for an action to be taken. Unlike an event's fire-and-forget approach, a command can fail as it is handled. The handler will then return an error. - -> Because we request an operation to be performed, command are written in imperative mood, such as `CreateFolderCommand`, and `DeletePlaylistCommand`. - -### Dispatch a command - -To dispatch a command, pass the `context.Context` and object to the `DispatchCtx` method: - -```go -// context.Context from caller -ctx := req.Request.Context() -cmd := &models.SendStickersCommand { - UserID: "taylor", - Count: 1, -} -if err := s.bus.DispatchCtx(ctx, cmd); err != nil { - if err == bus.ErrHandlerNotFound { - return nil - } - return err -} -``` - -> **Note:** `DispatchCtx` will return an error if no handler is registered for that command. - -> **Note:** `Dispatch` currently exists and requires no `context.Context` to be provided, but it's strongly suggested to not use this since there's an ongoing refactoring to remove usage of non-context-aware functions/methods and use context.Context everywhere. - -**Tip:** Browse the available commands in the `models` package. - -### Handle commands - -Let other parts of the application dispatch commands to a service, by registering a _command handler_: - -To handle a command, register a command handler in the `Init` function. - -```go -func (s *MyService) Init() error { - s.bus.AddHandlerCtx(s.SendStickers) - return nil -} - -func (s *MyService) SendStickers(ctx context.Context, cmd *models.SendStickersCommand) error { - // ... -} -``` - -> **Note:** The handler method may return an error if unable to complete the command. - -> **Note:** `AddHandler` currently exists and requires no `context.Context` to be provided, but it's strongly suggested to not use this since there's an ongoing refactoring to remove usage of non-context-aware functions/methods and use context.Context everywhere. - -## Queries - -A command handler can optionally populate the command sent to it. This pattern is commonly used to implement _queries_. - -### Making a query - -To make a query, dispatch the query instance just like you would a command. When the `DispatchCtx` method returns, the `Results` field contains the result of the query. - -```go -// context.Context from caller -ctx := req.Request.Context() -query := &models.FindDashboardQuery{ - ID: "foo", -} -if err := bus.Dispatch(ctx, query); err != nil { - return err -} -// The query now contains a result. -for _, item := range query.Results { - // ... -} -``` - -> **Note:** `Dispatch` currently exists and requires no `context.Context` to be provided, but it's strongly suggested to not use this since there's an ongoing refactoring to remove usage of non-context-aware functions/methods and use context.Context everywhere. - -### Return query results - -To return results for a query, set any of the fields on the query argument before returning: - -```go -func (s *MyService) FindDashboard(ctx context.Context, query *models.FindDashboardQuery) error { - // ... - query.Result = dashboard - return nil -} -``` diff --git a/contribute/architecture/backend/errors.md b/contribute/architecture/backend/errors.md new file mode 100644 index 00000000000..fa02090b6d8 --- /dev/null +++ b/contribute/architecture/backend/errors.md @@ -0,0 +1,81 @@ +# Errors + +Grafana introduced its own error type `github.com/grafana/grafana/pkg/util/errutil.Error` +in June 2022. It's built on top of the Go `error` interface extended to +contain all the information necessary by Grafana to handle errors in an +informative and safe way. + +Previously, Grafana has passed around regular Go errors and have had to +rely on bespoke solutions in API handlers to communicate informative +messages to the end-user. With the new `errutil.Error`, the API handlers +can be slimmed as information about public messaging, structured data +related to the error, localization metadata, log level, HTTP status +code, and so forth are carried by the error. + +## Basic use + +### Declaring errors + +For a service, declare the different categories of errors that may occur +from your service (this corresponds to what you might want to have +specific public error messages or their templates for) by globally +constructing variables using the `errutil.NewBase(status, messageID, opts...)` +function. + +The status code loosely corresponds to HTTP status codes and provides a +default log level for errors to ensure that the request logging is +properly informing administrators about various errors occurring in +Grafana (e.g. `StatusBadRequest` is generally speaking not as relevant +as `StatusInternal`). All available status codes live in the `errutil` +package and have names starting with `Status`. + +The messageID is constructed as `.` where +the `` corresponds to the root service directory per +[the package hierarchy](package-hierarchy.md) and `` +is a short identifier using dashes for word separation that identifies +the specific category of errors within the service. + +To set a static message sent to the client when the error occurs, the +`errutil.WithPublicMessage(message string)` option may be appended to +the NewBase function call. For dynamic messages or more options, refer +to the `errutil` package's GoDocs. + +Errors are then constructed using the `Base.Errorf` method, which +functions like the [fmt.Errorf](https://pkg.go.dev/fmt#Errorf) method +except that it creates an `errutil.Error`. + +```go +package main + +import ( + "errors" + "github.com/grafana/grafana/pkg/util/errutil" + "example.org/thing" +) + +var ErrBaseNotFound = errutil.NewBase(errutil.StatusNotFound, "main.not-found", errutil.WithPublicMessage("Thing not found")) + +func Look(id int) (*Thing, error) { + t, err := thing.GetByID(id) + if errors.Is(err, thing.ErrNotFound) { + return nil, ErrBaseNotFound.Errorf("did not find thing with ID %d: %w", id, err) + } + + return t, nil +} +``` + +Check out [errutil's GoDocs](https://pkg.go.dev/github.com/grafana/grafana@v0.0.0-20220621133844-0f4fc1290421/pkg/util/errutil) +for details on how to construct and use Grafana style errors. + +### Handling errors in the API + +API handlers use the `github.com/grafana/grafana/pkg/api/response.Err` +function to create responses based on `errutil.Error`s. + +> **Note:** (@sakjur 2022-06) `response.Err` requires all errors to be +> `errutil.Error` or it'll be considered an internal server error. +> This is something that should be fixed in the near future to allow +> fallback behavior to make it possible to correctly handle Grafana +> style errors if they're present but allow fallback to a reasonable +> default otherwise. diff --git a/contribute/architecture/backend/package-hierarchy.md b/contribute/architecture/backend/package-hierarchy.md index d940910cdd0..7f52da3682e 100644 --- a/contribute/architecture/backend/package-hierarchy.md +++ b/contribute/architecture/backend/package-hierarchy.md @@ -1,16 +1,217 @@ # Package hierarchy -The Go package hierarchy in Grafana should be organized logically (Ben Johnson's -[article](https://medium.com/@benbjohnson/standard-package-layout-7cdbc8391fc1) served as inspiration), according to the -following principles: +The Go packages in Grafana should be packaged by feature, keeping +packages as small as reasonable while retaining a clear sole ownership +of a single domain. -- Domain types and interfaces should be in "root" packages (not necessarily at the very top, of the hierarchy, but - logical roots) -- Sub-packages should depend on roots - sub-packages here typically contain implementations, for example of services +[Ben Johnson's standard package layout](https://medium.com/@benbjohnson/standard-package-layout-7cdbc8391fc1) serves as +inspiration for the way we organize packages. + +## Principles of how to structure a service in Grafana + +[](services.md) + +### Domain types and interfaces should be in local "root" packages + +Let's say you're creating a _tea pot_ service, place everything another +service needs to interact with the tea pot service in +_pkg/services/teapot_, choosing a name according to +[Go's package naming conventions](https://go.dev/blog/package-names). + +Typically, you'd have one or more interfaces that your service provides +in the root package along with any types, errors, and other constants +that makes sense for another service interacting with this service to +use. + +Avoid depending on other services when structuring the root package to +reduce the risk of running into circular dependencies. + +### Sub-packages should depend on roots, not the other way around + +Small-to-medium sized packages should be able to have only a single +sub-package containing the implementation of the service. By moving the +implementation into a separate package we reduce the risk of triggering +circular dependencies (in Go, circular dependencies are evaluated per +package and this structure logically moves it to be per type or function +declaration). + +Large packages may need utilize multiple sub-packages at the discretion +of the implementor. Keep interfaces and domain types to the root +package. + +### Try to name sub-packages for project wide uniqueness + +Prefix sub-packages with the service name or an abbreviation of the +service name (whichever is more appropriate) to provide an ideally +unique package name. This allows `teaimpl` to be distinguished from +`coffeeimpl` without the need for package aliases, and encourages the +use of the same name to reference your package throughout the codebase. + +### A well-behaving service provides test doubles for itself + +Other services may depend on your service, and it's good practice to +provide means for those services to set up a test instance of the +dependency as needed. Refer to +[Google Testing's Testing on the Toilet: Know Your Test Doubles](https://testing.googleblog.com/2013/07/testing-on-toilet-know-your-test-doubles.html) for a brief +explanation of how we semantically aim to differentiate fakes, mocks, +and stubs within our codebase. + +Place test doubles in a sub-package to your root package named +`test` or `test`, such that the `teapot` service may have the +`teapottest` or `teatest` + +A stub or mock may be sufficient if the service is not a dependency of a +lot of services or if it's called primarily for side effects so that a +no-op default behavior makes sense. + +Services which serve many other services and where it's feasible should +provide an in-memory backed test fake that can be used like the +regular service without the need of complicated setup. + +### Separate store and logic + +When building a new service, data validation, manipulation, scheduled +events and so forth should be collected in a service implementation that +is built to be agnostic about its store. + +The storage should be an interface that is not directly called from +outside the service and should be kept to a minimum complexity to +provide the functionality necessary for the service. + +A litmus test to reduce the complexity of the storage interface is +whether an in-memory implementation is a feasible test double to build +to test the service. + +### Outside the service root + +Some parts of the service definition remains outside the +service directory and reflects the legacy package hierarchy. +As of June 2022, the parts that remain outside the service are: + +#### Migrations + +`pkg/services/sqlstore/migrations` contains all migrations for SQL +databases, for all services (not including Grafana Enterprise). +Migrations are written per the [database.md](database.md#migrations) document. + +#### API endpoints + +`pkg/api/api.go` contains the endpoint definitions for the most of +Grafana HTTP API (not including Grafana Enterprise). ## Practical example -The `pkg/plugins` package contains plugin domain types, for example `DataPlugin`, and also interfaces -such as `RequestHandler`. Then you have the `pkg/plugins/managers` subpackage, which contains concrete implementations -such as the service `PluginManager`. The subpackage `pkg/plugins/backendplugin/coreplugin` contains `plugins.DataPlugin` -implementations. +The following is a simplified example of the package structure for a +service that doesn't do anything in particular. + +None of the methods or functions are populated and in practice most +packages will consist of multiple files. There isn't a Grafana-wide +convention for which files should exist and contain what. + +`pkg/services/alphabetical` + +``` +package alphabetical + +type Alphabetical interface { + // GetLetter returns either an error or letter. + GetLetter(context.Context, GetLetterQuery) (Letter, error) + // ListCachedLetters cannot fail, and doesn't return an error. + ListCachedLetters(context.Context, ListCachedLettersQuery) Letters + // DeleteLetter doesn't have any return values other than errors, so it + // returns only an error. + DeleteLetter(context.Contxt, DeltaCommand) error +} + +type Letter byte + +type Letters []Letter + +type GetLetterQuery struct { + ID int +} + +// Create queries/commands for methods even if they are empty. +type ListCachedLettersQuery struct {} + +type DeleteLetterCommand struct { + ID int +} + +``` + +`pkg/services/alphabetical/alphabeticalimpl` + +``` +package alphabeticalimpl + +// this name can be whatever, it's not supposed to be used from outside +// the service except for in Wire. +type Svc struct { … } + +func ProviceSvc(numbers numerical.Numerical, db db.DB) Svc { … } + +func (s *Svc) GetLetter(ctx context.Context, q root.GetLetterQuery) (root.Letter, error) { … } +func (s *Svc) ListCachedLetters(ctx context.Context, q root.ListCachedLettersQuery) root.Letters { … } +func (s *Svc) DeleteLetter(ctx context.Context, q root.DeleteLetterCommand) error { … } + +type letterStore interface { + Get(ctx.Context, id int) (root.Letter, error) + Delete(ctx.Context, root.DeleteLetterCommand) error +} + +type sqlLetterStore struct { + db.DB +} + +func (s *sqlStore) Get(ctx.Context, id int) (root.Letter, error) { … } +func (s *sqlStore) Delete(ctx.Context, root.DeleteLetterCommand) error { … } +``` + +## Legacy package hierarchy + +> **Note:** A lot of services still adhere to the legacy model as outlined below. While it is ok to +> extend existing services based on the legacy model, you are _strongly_ encouraged to structure any +> new services or major refactorings using the new package layout. + +Grafana has long used a package-by-layer layout where domain types +are placed in **pkg/models**, all SQL logic in **pkg/services/sqlstore**, +and so forth. + +This is an example of how the _tea pot_ service could be structured +throughout the codebase in the legacy model. + +- _pkg/_ + - _api/_ + - _api.go_ contains the endpoints for the + - _tea_pot.go_ contains methods on the _pkg/api.HTTPServer_ type + that interacts with the service based on queries coming in via the HTTP + API. + - _dtos/tea_pot.go_ extends the _pkg/models_ file with types + that are meant for translation to and from the API. It's not as commonly + present as _pkg/models_. + - _models/tea_pot.go_ contains the models for the service, this + includes the _command_ and _query_ structs that are used when calling + the service or SQL store methods related to the service and also any + models representing an abstraction provided by the service. + - _services/_ + - _sqlstore_ + - _tea_pot.go_ contains SQL queries for + interacting with stored objects related to the tea pot service. + - _migrations/tea_pot.go_ contains the migrations necessary to + build the + - _teapot/\*_ contains functions or a service for doing + logical operations beyond those done in _pkg/api_ or _pkg/services/sqlstore_ + for the service. + +The implementation of legacy services varies widely from service to +service, some or more of these files may be missing and there may be +more files related to a service than those listed here. + +Some legacy services providing infrastructure will also take care of the +integration with several domains. The cleanup service both +provides the infrastructure to occasionally run cleanup scripts and +defines the cleanup scripts. Ideally, this would be migrated +to only handle the scheduling and synchronization of clean up jobs. +The logic for the individual jobs would be placed with a service that is +related to whatever is being cleaned up.