From b26b9226582824c7c789fbd817064685eb7843fe Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Fri, 2 Jul 2021 17:07:21 +0200 Subject: [PATCH] Contribute: Ongoing refactoring of context.Context everywhere (#36363) --- .../architecture/backend/communication.md | 28 +++++++++++++------ contribute/architecture/backend/database.md | 10 +++---- contribute/architecture/backend/services.md | 8 ++++-- pkg/README.md | 15 +++++++++- 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/contribute/architecture/backend/communication.md b/contribute/architecture/backend/communication.md index 8d873482c68..bd1d9300fb4 100644 --- a/contribute/architecture/backend/communication.md +++ b/contribute/architecture/backend/communication.md @@ -12,7 +12,7 @@ An event is something that happened in the past. Since an event has already happ ### Subscribe to an event -In order to react to an event, you first need to _subscribe_ to it. +In order to react to an event, you first need to _subscribe_ to it. To subscribe to an event, register an _event listener_ in the service's `Init` method: @@ -51,14 +51,16 @@ A command is a request for an action to be taken. Unlike an event's fire-and-for ### Dispatch a command -To dispatch a command, pass the object to the `Dispatch` method: +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.Dispatch(cmd); err != nil { +if err := s.bus.DispatchCtx(ctx, cmd); err != nil { if err == bus.ErrHandlerNotFound { return nil } @@ -66,7 +68,9 @@ if err := s.bus.Dispatch(cmd); err != nil { } ``` -> **Note:** `Dispatch` will return an error if no handler is registered for that command. +> **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. @@ -78,30 +82,34 @@ To handle a command, register a command handler in the `Init` function. ```go func (s *MyService) Init() error { - s.bus.AddHandler(s.SendStickers) + s.bus.AddHandlerCtx(s.SendStickers) return nil } -func (s *MyService) SendStickers(cmd *models.SendStickersCommand) error { +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 `Dispatch` method returns, the `Results` field contains the result of the 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(query); err != nil { +if err := bus.DispatchCtx(ctx, query); err != nil { return err } // The query now contains a result. @@ -110,12 +118,14 @@ 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(query *models.FindDashboardQuery) error { +func (s *MyService) FindDashboard(ctx context.Context, query *models.FindDashboardQuery) error { // ... query.Result = dashboard return nil diff --git a/contribute/architecture/backend/database.md b/contribute/architecture/backend/database.md index 365109e54fa..75f7a63704b 100644 --- a/contribute/architecture/backend/database.md +++ b/contribute/architecture/backend/database.md @@ -33,18 +33,18 @@ To register a handler: ```go func init() { - bus.AddHandler("sql", DeleteDashboard) + bus.AddHandlerCtx("sql", DeleteDashboard) } -func DeleteDashboard(cmd *models.DeleteDashboardCommand) error { - return inTransaction(func(sess *DBSession) error { +func DeleteDashboard(ctx context.Context, cmd *models.DeleteDashboardCommand) error { + return inTransactionCtx(ctx, func(sess *DBSession) error { _, err := sess.Exec("DELETE FROM dashboards WHERE dashboard_id=?", cmd.DashboardID) return err }) } ``` -Here, `inTransaction` is a helper function in the `sqlstore` package that provides a session, that lets you execute SQL statements. +Here, `inTransactionCtx` is a helper function in the `sqlstore` package that provides a session, that lets you execute SQL statements. ## `SQLStore` @@ -61,7 +61,7 @@ type MyService struct { You can now make SQL queries in any of your [command handlers](communication.md#handle-commands) or [event listeners](communication.md#subscribe-to-an-event): ```go -func (s *MyService) DeleteDashboard(cmd *models.DeleteDashboardCommand) error { +func (s *MyService) DeleteDashboard(ctx context.Context, cmd *models.DeleteDashboardCommand) error { if err := s.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { _, err := sess.Exec("DELETE FROM dashboards WHERE dashboard_id=?", cmd.DashboardID) return err diff --git a/contribute/architecture/backend/services.md b/contribute/architecture/backend/services.md index cb36b578c2d..d428de6258c 100644 --- a/contribute/architecture/backend/services.md +++ b/contribute/architecture/backend/services.md @@ -1,6 +1,6 @@ # Services -A Grafana _service_ encapsulates and exposes application logic to the rest of the application, through a set of related operations. +A Grafana _service_ encapsulates and exposes application logic to the rest of the application, through a set of related operations. Before a service can start communicating with the rest of Grafana, it needs to be registered in the _service registry_. @@ -48,7 +48,7 @@ import _ "github.com/grafana/grafana/pkg/services/mysvc" ## Dependencies -Grafana uses the [inject](https://github.com/facebookgo/inject) package to inject dependencies during runtime. +Grafana uses the [inject](https://github.com/facebookgo/inject) package to inject dependencies during runtime. For example, to access the [bus](communication.md), add it to the `MyService` struct: @@ -67,3 +67,7 @@ type MyService struct { ``` > **Note:** Any injected dependency needs to be an exported field. Any unexported fields result in a runtime error. + +## Methods + +Any public method of a service should take `context.Context` as its first argument. If the method calls the bus, other services or the database the context should be propagated, if possible. diff --git a/pkg/README.md b/pkg/README.md index b7afa3412c6..9e345df8471 100644 --- a/pkg/README.md +++ b/pkg/README.md @@ -40,7 +40,7 @@ These issues are not something we want to address all at once but something we w ### Global state -Global state makes testing and debugging software harder and it's something we want to avoid when possible. Unfortunately, there is quite a lot of global state in Grafana. +Global state makes testing and debugging software harder and it's something we want to avoid when possible. Unfortunately, there is quite a lot of global state in Grafana. We want to migrate away from this by using the `inject` package to wire up all dependencies either in `pkg/cmd/grafana-server/main.go` or self-registering using `registry.RegisterService` ex https://github.com/grafana/grafana/blob/main/pkg/services/cleanup/cleanup.go#L25. @@ -84,3 +84,16 @@ All new features that require state should be possible to configure using config - [Dashboards](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/dashboards) Today its only possible to provision data sources and dashboards but this is something we want to support all over Grafana. + +### Use context.Context "everywhere" + +The package [context](https://golang.org/pkg/context/) should be used and propagated through all the layers of the code. For example the `context.Context` of an incoming API request should be propagated to any other layers being used such as the bus, service and database layers. Utility functions/methods normally doesn't need `context.Context` To follow best practices, any function/method that receives a context.Context argument should receive it as its first argument. + +To be able to solve certain problems and/or implement and support certain features making sure that `context.Context` is passed down through all layers of the code is vital. Being able to provide contextual information for the full life-cycle of an API request allows us to use contextual logging, provide contextual information about the authenticated user, create multiple spans for a distributed trace of service calls and database queries etc. + +Code should use `context.TODO` when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a `context.Context` argument). + + +More details in [Services](/contribute/architecture/backend/services.md), [Communication](/contribute/architecture/backend/communication.md) and [Database](/contribute/architecture/backend/database.md). + +[Original design doc](https://docs.google.com/document/d/1ebUhUVXU8FlShezsN-C64T0dOoo-DaC9_r-c8gB2XEU/edit#).