IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-01-27 16:57:14 -06:00
Code Issues Packages Projects Releases Wiki Activity

Docs: Consolidate backend guidelines (#19823)

Browse Source 
* Consolidate backend guidelines

* Fix link

* Fix review comments
This commit is contained in:
Marcus Olsson 2019-10-30 13:59:27 -07:00 committed by GitHub
parent 861eb72113
commit 69906f73a2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 81 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
contribute/style-guides/backend.md
View File

@ -1,27 +1,45 @@
# Backend style guide
Grafanas backend has been developed for a long time with a mix of code styles.
Grafanas backend has been developed for a long time with a mix of code styles. This guide explains how we want to write Go code in the future.
This style guide is a guide for how we want to write Go code in the future. Generally, we want to follow the style guides used in Go [Code Review Comments](https://code.google.com/p/go-wiki/wiki/CodeReviewComments) and Peter Bourgon's [Go: Best Practices for Production Environments](http://peter.bourgon.org/go-in-production/#formatting-and-style)
Unless stated otherwise, use the guidelines listed in the following articles:
- [Effective Go](https://golang.org/doc/effective_go.html)
- [Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
- [Go: Best Practices for Production Environments](http://peter.bourgon.org/go-in-production/#formatting-and-style)
## Linting and formatting
We enforce strict `gofmt` formatting and use some linters on our codebase. You can lint the codebase with <akefile -
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/master/conf/revive.toml)
- [GolangCI-Lint](https://github.com/golangci/golangci-lint)
- [gosec](https://github.com/securego/gosec)
To run all linters, use the `lint-go` Makefile target:
```bash
$ make lint-go
make lint-go
```
We use [revive](https://github.com/mgechev/revive) as a go linter, and do enforce our [custom config](/conf/revive.toml) for it.
The end goal is to follow the golint. And the approuch for reachin that goal is to lint all parts of the codebase that we are currently working on and enable stricter linting for more areas as we go.
## Testing
We use GoConvey for BDD/scenario based testing. Which we think is useful for testing certain chain or interactions. Ex https://github.com/grafana/grafana/blob/master/pkg/services/auth/auth_token_test.go
We value clean & readable code that is loosely coupled and covered by unit tests. This makes it easier to collaborate and maintain the code. 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.
We value clean and readable code, that is loosely coupled and covered by unit tests. This makes it easier to collaborate and maintain the code.
For new tests its preferred to use standard library testing.
Tests must use the standard library, `testing`. For assertions, prefer using [testify](https://github.com/stretchr/testify).
### Mocks/Stubs
As a general rule of thumb we try to override/replace functions/methods when mocks/stubs are needed. One common task is the need of overriding time (`time.Now()`). See usage of `getTime` variable in [code](https://github.com/grafana/grafana/blob/52c39904120fb0b98494b961be67bb47574245b1/pkg/services/auth/auth_token.go#L22) and in [test](https://github.com/grafana/grafana/blob/52c39904120fb0b98494b961be67bb47574245b1/pkg/services/auth/auth_token_test.go#L23-L26) as an example.
The majority of our tests uses [GoConvey](http://goconvey.co/) but that's something we want to avoid going forward.
When you need to stub/mock an interface you can implement a struct that allows you to override methods on a test-by-test basis. See [stub](https://github.com/grafana/grafana/blob/52c39904120fb0b98494b961be67bb47574245b1/pkg/services/auth/testing.go) and [example usage](https://github.com/grafana/grafana/blob/52c39904120fb0b98494b961be67bb47574245b1/pkg/middleware/middleware_test.go#L153-L180).
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.
## General guidelines
- Avoid using import aliases, e.g. `import m "github.com/grafana/grafana/pkg/models"`.

48
pkg/ARCHITECTURE.md
View File

@ -1,48 +0,0 @@
# Backend code structure
Grafanas backend is written in GO using sqlite3/mysql or postgres as storage for dashboards, users etc. When Grafana was born there didnt exist much guides or direction for how to write medium sized application. So there are parts of Grafana code base that didn't quite pan out as we wanted. More about that under current rewrites! :)
## Central folders of Grafanas 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 assosicated with a struct that refers to all depedencies. Ex bus. |
| /pkg/cmd | The binaries that we build. Grafana-server and grafana-cli. |
| /pkg/components | Mixed content of packages that we copied into Grafana and packages we implemented ourself. The Purpose of this folders should be packages that are too big for util and doesnt belong somewhere else. |
| /pkg/infra | Packages in infra should be packages that are used in multiple places of Grafana without knowing anything about Grafana domain. E.g. Logs, Metrics, Traces. |
| /pkg/services | Packages in services are responsible for peristing domain objects and manage the relationship between domain objects. Services should communicate with each other using DI when possible. Most part of Grafana's codebase still relay on Global state for this. Any new features going forwards should use DI. |
| /pkg/tsdb | All backend implementations of the data sources in Grafana. Used by both Grafanas 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 Grafanas backend
| package | description |
| ------- | ----------- |
| /pkg/bus | The bus! Described more below. |
| /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 refs within Grafana but that is something we should avoid going forward). |
| /pkg/registry | service management package. |
| /pkg/services/alerting | Grafanas alerting services. The alerting engine run in a seperate go routine and should not depend on anything else within Grafana. |
| /pkg/services/sqlstore | Currently where are database calls resides. |
| /pkg/setting | settings package for Grafana. Anything related to grafana global configuration should be dealt with in this package. |
## Testing
We value clean & readable code that is loosely coupled and covered by unit tests. This makes it easier to collaborate and maintain the code. 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.
The majority of our tests uses go convey but thats something we want to avoid going forward.
For new tests we want to use standard library and `testify/assert`.
## Import aliases
Currently there are import aliases for `/pkg/models` package but thats something we want to avoid going forward.
Newly introduced code should refer explicitly to the `models` instead of using the alias `m`. Whenever changing existing code it's desired to remove the import aliases as well.
## The Bus
The bus is our way to introduce indirection between the HTTP handlers and sqlstore (responsible for fetching data from the database). Http handlers and sqlstore don't depend on each other. They only depend on the bus and the domain model(pkg/models). This makes it easier to test the code and avoids coupling. More about this under `current rewrite/refactorings`
## Services/Repositories
Services within Grafana should be self-contained and only talk to other parts of Grafana using the bus or repositories that have been made available through Grafana service registry. All services should register themselves to the `registry` package in an init function. Only registration should be done in the init function. Init functions should be avoided as much as possible.
When Grafana starts all init functions within the services will be called and register themselves.
Grafana will then create a Graph of all dependencies and inject the services that other services depend on. This is solved with [inject library](https://github.com/facebookgo/inject) in https://github.com/grafana/grafana/blob/master/pkg/cmd/grafana-server/server.go#L75
## Provisionable*
All new features that require state should be possible to configure using config files. Ex [data sources](https://github.com/grafana/grafana/tree/master/pkg/services/provisioning/datasources), [alert notifiers](https://github.com/grafana/grafana/tree/master/pkg/services/provisioning/notifiers), [dashboards](https://github.com/grafana/grafana/tree/master/pkg/services/provisioning/dashboards), teams etc. Today its only possible to provision data sources and dashboards but this is something we want to support all over Grafana.

62
pkg/README.md
View File

@ -1,46 +1,82 @@
# Grafana backend codebase
# Backend
The code should follow the [Backend style guide](/contribute/style-guides/backend.md).
This directory contains the code for the Grafana backend. This document gives an overview of the directory structure, and ongoing refactorings.
Refer to [architecture](ARCHITECTURE.md) for a description of the backend codebase architecture.
For more information on developing for the backend:
- [Backend style guide](/contribute/style-guides/backend.md)
- [Architecture](/contribute/architecture)
## 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 peristing 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/architecture/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. |
## Dependency management
Refer to [UPGRADING_DEPENDENCIES.md](https://github.com/grafana/grafana/blob/master/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 master 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
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/master/pkg/services/cleanup/cleanup.go#L25.
## Limit the use of the init() function
### Limit the use of the init() function
Only use the init() function to register services/implementations.
## Settings refactoring
### 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
### 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 [ARCHITECTURE.MD](ARCHITECTURE.md#Testing).
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/style-guides/backend.md).
## Refactor SqlStore
### Refactor SqlStore
The `sqlstore` handlers all use a global xorm engine variable. Refactor them to use the `SqlStore` instance.
## Avoid global HTTP handler functions
### 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
### 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.
## Dependency management
### Provisionable*
Refer to [UPGRADING_DEPENDENCIES.md](https://github.com/grafana/grafana/blob/master/UPGRADING_DEPENDENCIES.md).
All new features that require state should be possible to configure using config files. For example:
- [Data sources](https://github.com/grafana/grafana/tree/master/pkg/services/provisioning/datasources)
- [Alert notifiers](https://github.com/grafana/grafana/tree/master/pkg/services/provisioning/notifiers)
- [Dashboards](https://github.com/grafana/grafana/tree/master/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.