apitech
/
grafana
mirror of https://github.com/grafana/grafana
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Docs: Update backend contribution guidelines (#63034)

Browse Source
pull/63052/head
Emil Tullstedt 2 years ago committed by GitHub
parent 06dc965610
commit 4fd7743b81
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 196 additions and 115 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 103
      contribute/backend/README.md
  2. 3
      contribute/backend/communication.md
  3. 169
      contribute/backend/recommended-practices.md
  4. 36
      contribute/backend/style-guide.md

103
contribute/backend/README.md
Unescape View File

@ -1,98 +1,23 @@
# Backend
This document gives an overview of the directory structure, and ongoing refactorings.
First read the [backend style guide](/contribute/backend/style-guide.md)
to get a sense for how we work to ensure that the Grafana codebase is
consistent and accessible. The rest of the backend contributor
documentation is more relevant to reviewers and contributors looking to
make larger changes.
For more information on developing for the backend:
For anyone reviewing code for Grafana's backend, a basic understanding
of content of the following files is expected:
- [Backend style guide](/contribute/backend/style-guide.md)
- [Architecture](/contribute/architecture)
- [Currently recommended practices](/contribute/backend/recommended-practices.md)
- [Services](/contribute/backend/services.md)
- [Communication](/contribute/backend/communication.md)
- [Database](/contribute/backend/database.md)
## Central folders of Grafana's backend
| folder | description |
| --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| /pkg/api | HTTP handlers and routing. Almost all handler funcs are global which is something we would like to improve in the future. Handlers should be associated with a struct that refers to all dependencies. |
| /pkg/cmd | The binaries that we build: grafana-server and grafana-cli. |
| /pkg/components | A mix of third-party packages and packages we have implemented ourselves. Includes our packages that have out-grown the util package and don't naturally belong somewhere else. |
| /pkg/infra | Packages in infra should be packages that are used in multiple places in Grafana without knowing anything about the Grafana domain. |
| /pkg/services | Packages in services are responsible for persisting domain objects and manage the relationship between domain objects. Services should communicate with each other using DI when possible. Most of Grafana's codebase still relies on global state for this. Any new features going forward should use DI. |
| /pkg/tsdb | All backend implementations of the data sources in Grafana. Used by both Grafana's frontend and alerting. |
| /pkg/util | Small helper functions that are used in multiple parts of the codebase. Many functions are placed directly in the util folders which is something we want to avoid. Its better to give the util function a more descriptive package name. Ex `errutil`. |
## Central components of Grafana's backend
| package | description |
| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| /pkg/bus | The bus is described in more details under [Communication](/contribute/backend/communication.md) |
| /pkg/models | This is where we keep our domain model. This package should not depend on any package outside standard library. It does contain some references within Grafana but that is something we should avoid going forward. |
| /pkg/registry | Package for managing services. |
| /pkg/services/alerting | Grafana's alerting services. The alerting engine runs in a separate goroutine and shouldn't depend on anything else within Grafana. |
| /pkg/services/sqlstore | Currently where the database logic resides. |
| /pkg/setting | Anything related to Grafana global configuration should be dealt with in this package. |
Reviewers who review large changes should additionally make a habit out
of familiarizing themselves with the content of
[/contribute/backend](/contribute/backend) from time to time.
## Dependency management
Refer to [UPGRADING_DEPENDENCIES.md](https://github.com/grafana/grafana/blob/main/UPGRADING_DEPENDENCIES.md).
## Ongoing refactoring
These issues are not something we want to address all at once but something we will improve incrementally. Since Grafana is released at a regular schedule the preferred approach is to do this in batches. Not only is it easier to review, but it also reduces the risk of conflicts when cherry-picking fixes from main to release branches. Please try to submit changes that span multiple locations at the end of the release cycle. We prefer to wait until the end because we make fewer patch releases at the end of the release cycle, so there are fewer opportunities for complications.
### 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.
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.
### Limit the use of the init() function
Only use the init() function to register services/implementations.
### Settings refactoring
The plan is to move all settings to from package level vars in settings package to the [setting.Cfg](https://github.com/grafana/grafana/blob/df917663e6f358a076ed3daa9b199412e95c11f4/pkg/setting/setting.go#L210) struct. To access the settings, services and components can inject this setting.Cfg struct:
[Cfg struct](https://github.com/grafana/grafana/blob/df917663e6f358a076ed3daa9b199412e95c11f4/pkg/setting/setting.go#L210)
[Injection example](https://github.com/grafana/grafana/blob/df917663e6f358a076ed3daa9b199412e95c11f4/pkg/services/cleanup/cleanup.go#L20)
### Reduce the use of GoConvey
We want to migrate away from using GoConvey. Instead, we want to use stdlib testing, because it's the most common approach in the Go community and we think it will be easier for new contributors. Read more about how we want to write tests in the [style guide](/contribute/backend/style-guide.md).
### Refactor SqlStore
The `sqlstore` handlers all use a global xorm engine variable. Refactor them to use the `SqlStore` instance.
### Avoid global HTTP handler functions
Refactor HTTP handlers so that the handler methods are on the HttpServer instance or a more detailed handler struct. E.g (AuthHandler). This ensures they get access to HttpServer service dependencies (and Cfg object) and can avoid global state.
### Date comparison
Store newly introduced date columns in the database as epochs if they require date comparison. This permits a unified approach for comparing dates against all the supported databases instead of handling dates differently for each database. Also, by comparing epochs, we no longer need error pruning transformations to and from other time zones.
### Avoid use of the simplejson package
Use of the `simplejson` package (`pkg/components/simplejson`) in place of types (Go structs) results in code that is difficult to maintain. Instead, create types for objects and use the Go standard library's [`encoding/json`](https://golang.org/pkg/encoding/json/) package.
### Provisionable\*
All new features that require state should be possible to configure using config files. For example:
- [Data sources](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/datasources)
- [Alert notifiers](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/notifiers)
- [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/backend/services.md), [Communication](/contribute/backend/communication.md) and [Database](/contribute/backend/database.md).
[Original design doc](https://docs.google.com/document/d/1ebUhUVXU8FlShezsN-C64T0dOoo-DaC9_r-c8gB2XEU/edit#).

3
contribute/backend/communication.md
Unescape View File

@ -1,6 +1,7 @@
# Communication
Grafana uses a _bus_ to pass messages between different parts of the application. All communication over the bus happens synchronously.
Grafana use dependency injection and method calls on Go interfaces to
communicate between different parts of the backend.
## Commands and queries

169
contribute/backend/recommended-practices.md
Unescape View File

@ -0,0 +1,169 @@
# Currently recommended practices
We occasionally identify patterns that are either useful or harmful that
we'll want to introduce or remove from the codebase. When the complexity
or importance of introducing or removing such a pattern is sufficiently
high, we'll document it here to provide an addressable local
'currently recommended practice'. By collecting these practices in a
single place, we're able to reference them and make it easier to have a
shared understanding of how to write idiomatic code for the Grafana
backend.
Large-scale refactoring based on a new recommended practice is a
delicate matter, and most of the time it's better to introduce the new
way incrementally over multiple releases and over time to balance the
want to introduce new useful patterns and the need to keep Grafana
stable. It's also easier to review and revert smaller chunks of changes,
reducing the risk of complications.
| State | Description |
| ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- |
| Proposed | The practice has been proposed and been positively received by the Grafana team. Following the proposal is a discretionary choice for developers. |
| Ongoing, active | The practice is actively being worked on. New code should adhere to the practice where at all possible. |
| Ongoing, passive | There is no immediate active work on refactoring old code. New code should adhere to the practice where at all possible. |
| Completed | The work has been done and there is no, or negligible, legacy code left that need refactoring. New code must adhere to the practice. |
| Abandoned | The practice has no longer any active ongoing work and new code don't need to comply with the practice described. |
## 1 - Idiomatic Grafana code should be idiomatic Go code
**Status:** Ongoing, passive.
It'll be easier for contributors to start contributing to Grafana if our
code is easily understandable. When there isn't a more specific Grafana
recommended practice, we recommend following the practices as put forth
by the Go project for development of Go code or the Go compiler itself
when applicable.
The first resource we recommend everyone developing Grafana's backend to
skim is "[Effective Go](https://golang.org/doc/effective_go.html)",
which isn't updated to reflect more recent changes since Go was
initially released but remain a good source for understanding the
general differences between Go and other languages.
Secondly, the guidelines for [Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
for the Go compiler can mostly be applied directly to the Grafana
codebase. There are idiosyncrasies in Grafana such as interfaces living
closer to its declaration than to its users for services and don't
enforce documentation of public declarations (prioritize high coverage
of documentation aimed at end-users over documenting internals in the
backend).
- [Effective Go](https://golang.org/doc/effective_go.html)
- [Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
## 100 - Global state
**State:** Ongoing, passive.
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
[Wire](https://github.com/google/wire) and dependency injection to pack
## 101 - Limit the use of the init() function
**State:** Ongoing, passive.
Only use the init() function to register services/implementations.
## 102 - Settings refactoring
**State:** Ongoing, passive.
The plan is to move all settings to from package level vars in settings package to the [setting.Cfg](https://github.com/grafana/grafana/blob/df917663e6f358a076ed3daa9b199412e95c11f4/pkg/setting/setting.go#L210) struct. To access the settings, services and components can inject this setting.Cfg struct:
- [Cfg struct](https://github.com/grafana/grafana/blob/df917663e6f358a076ed3daa9b199412e95c11f4/pkg/setting/setting.go#L210)
- [Injection example](https://github.com/grafana/grafana/blob/c9773e55b234b7637ea97b671161cd856a1d3d69/pkg/services/cleanup/cleanup.go#L34)
## 103 - Reduce the use of GoConvey
**State:** Completed.
We want to migrate away from using GoConvey. Instead, we want to use
stdlib testing with [testify](https://github.com/stretchr/testify),
because it's the most common approach in the Go community, and we think
it will be easier for new contributors. Read more about how we want to
write tests in the [style guide](/contribute/backend/style-guide.md).
## 104 - Refactor SqlStore
**State:** Completed.
The `sqlstore` handlers all use a global xorm engine variable. Refactor them to use the `SqlStore` instance.
## 105 - Avoid global HTTP handler functions
**State:** Ongoing, passive.
Refactor HTTP handlers so that the handler methods are on the HttpServer instance or a more detailed handler struct. E.g (AuthHandler). This ensures they get access to HttpServer service dependencies (and Cfg object) and can avoid global state.
## 106 - Date comparison
**State:** Ongoing, passive.
Store newly introduced date columns in the database as epoch based
integers (i.e. unix timestamps) if they require date comparison. This
permits a unified approach for comparing dates against all the supported
databases instead of handling dates differently for each database. Also,
by comparing epoch based integers, we no longer need error pruning
transformations to and from other time zones.
## 107 - Avoid use of the simplejson package
**State:** Ongoing, passive
Use of the `simplejson` package (`pkg/components/simplejson`) in place
of types (Go structs) results in code that is difficult to maintain.
Instead, create types for objects and use the Go standard library's
[`encoding/json`](https://golang.org/pkg/encoding/json/) package.
## 108 - Provisionable\*
**State:** Abandoned: Grafana's file based refactoring is limited to work natively only on on-premise installations of Grafana. We're looking at enhancing the use of the API to enable provisioning for all Grafana instances.
All new features that require state should be possible to configure using config files. For example:
- [Data sources](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/datasources)
- [Alert notifiers](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/notifiers)
- [Dashboards](https://github.com/grafana/grafana/tree/main/pkg/services/provisioning/dashboards)
Today it's only possible to provision data sources and dashboards but this is something we want to support all over Grafana.
### 109 - Use context.Context "everywhere"
**State:** Completed.
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 Go best practices, any function/method that receives a
[`context.Context` argument should receive it as its first parameter](https://github.com/golang/go/wiki/CodeReviewComments#contexts).
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/backend/services.md), [Communication](/contribute/backend/communication.md) and [Database](/contribute/backend/database.md).
[Original design doc](https://docs.google.com/document/d/1ebUhUVXU8FlShezsN-C64T0dOoo-DaC9_r-c8gB2XEU/edit#).
## 110 - Move API error handling to service layer
**State:** Ongoing, passive.
All errors returned from Grafana's services should carry a status and
the information necessary to provide a structured end-user facing
message that the frontend can display and internationalize for
end-users.
More details in [Errors](/contribute/backend/errors.md).

36
contribute/backend/style-guide.md
Unescape View File

@ -10,19 +10,12 @@ Unless stated otherwise, use the guidelines listed in the following articles:
## Linting and formatting
To ensure consistency across the Go codebase, we require all code to pass a number of linter checks.
To ensure consistency across the Go codebase, we require all code to
pass a number of linter checks.
We use the standard following linters:
- [gofmt](https://golang.org/cmd/gofmt/)
- [golint](https://github.com/golang/lint)
- [go vet](https://golang.org/cmd/vet/)
In addition to the standard linters, we also use:
- [revive](https://revive.run/) with a [custom config](https://github.com/grafana/grafana/blob/main/conf/revive.toml)
- [GolangCI-Lint](https://github.com/golangci/golangci-lint)
- [gosec](https://github.com/securego/gosec)
We use [GolangCI-Lint](https://github.com/golangci/golangci-lint) with a
custom configuration [.golangci.toml](/.golangci.toml) to run these
checks.
To run all linters, use the `lint-go` Makefile target:
@ -36,10 +29,6 @@ We value clean and readable code, that is loosely coupled and covered by unit te
Tests must use the standard library, `testing`. For assertions, prefer using [testify](https://github.com/stretchr/testify).
The majority of our tests uses [GoConvey](http://goconvey.co/) but that's something we want to avoid going forward.
In the `sqlstore` package we do database operations in tests and while some might say that's not suited for unit tests. We think they are fast enough and provide a lot of value.
### Integration Tests
We run unit and integration tests separately, to help keep our CI pipeline running smoothly and provide a better developer experience.
@ -55,7 +44,8 @@ func TestIntegrationFoo(t *testing.T) {
}
```
If you do not follow this convention, your integration test may be run twice or not run at all.
> Warning
> If you do not follow this convention, your integration test may be run twice or not run at all.
### Assertions
@ -72,8 +62,7 @@ code, plus lets you run each test case in isolation when debugging. Don't use `t
### Cleanup
Use [`t.Cleanup`](https://golang.org/pkg/testing/#T.Cleanup) to clean up resources in tests. It's a less fragile choice than `defer`, since it's independent of which
function you call it in. It will always execute after the test is over in reverse call order (last `t.Cleanup` first, same as `defer`).
Use [`t.Cleanup`](https://golang.org/pkg/testing/#T.Cleanup) to clean up resources in tests. It's a preferable to `defer`, as it can be called from helper functions. It will always execute after the test is over in reverse call order (last `t.Cleanup` first, same as `defer`).
### Mock
@ -203,9 +192,6 @@ If a column, or column combination, should be unique, add a corresponding unique
## JSON
The simplejson package is used a lot throughout the backend codebase, but it's legacy, so if at all possible
avoid using it in new code. Use [json-iterator](https://github.com/json-iterator/go) instead, which is a more performant
drop-in alternative to the standard [encoding/json](https://golang.org/pkg/encoding/json/) package. While encoding/json
is a fine choice, profiling shows that json-iterator may be 3-4 times more efficient for encoding. We haven't profiled
its parsing performance yet, but according to json-iterator's own benchmarks, it appears even more superior in this
department.
The simplejson package is used a lot throughout the backend codebase,
but it's legacy, so if at all possible avoid using it in new code.
Use [encoding/json](https://golang.org/pkg/encoding/json/) instead.

Write Preview
Loading…
Cancel
Save