codestyle: styleguide and arch for grafanas backend (#17545)

CONTRIBUTING.md

@@ -1,18 +1,19 @@
-
 # Contributing
 
 Grafana uses GitHub to manage contributions.
 Contributions take the form of pull requests that will be reviewed by the core team.
 
-* If you are a new contributor see: [Steps to Contribute](#steps-to-contribute)
+- If you are a new contributor see: [Steps to Contribute](#steps-to-contribute)
 
-* If you have a trivial fix or improvement, go ahead and create a pull request.
+- If you have a trivial fix or improvement, go ahead and create a pull request.
 
-* If you plan to do something more involved, discuss your idea on the respective [issue](https://github.com/grafana/grafana/issues) or create a [new issue](https://github.com/grafana/grafana/issues/new) if it does not exist. This will avoid unnecessary work and surely give you and us a good deal of inspiration.
+- If you plan to do something more involved, discuss your idea on the respective [issue](https://github.com/grafana/grafana/issues) or create a [new issue](https://github.com/grafana/grafana/issues/new) if it does not exist. This will avoid unnecessary work and surely give you and us a good deal of inspiration.
 
-* Sign our [CLA](http://docs.grafana.org/contribute/cla/).
+- Sign our [CLA](http://docs.grafana.org/contribute/cla/).
 
-* For changes in the backend, 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)
+- Make sure to follow the code style guides
+  - [Backend](https://github.com/grafana/grafana/tree/master/pkg)
+  - [Frontend](https://github.com/grafana/grafana/tree/master/style_guides)
 
 ## Steps to Contribute
 
@@ -22,84 +23,40 @@ Please check the [`beginner friendly`](https://github.com/grafana/grafana/issues
 
 To setup a local development environment we recommend reading [Building Grafana from source](http://docs.grafana.org/project/building_from_source/)
 
-
 ## Pull Request Checklist
 
-* Branch from the master branch and, if needed, rebase to the current master branch before submitting your pull request. If it doesn't merge cleanly with master you may be asked to rebase your changes.
-
-* If your patch is not getting reviewed or you need a specific person to review it, you can @-reply a reviewer asking for a review in the pull request or a comment.
-
-* Add tests relevant to the fixed bug or new feature.
-
-* Follow [PR and commit messages guidelines](#PR-and-commit-messages-guidelines)
-
-### Pull requests with new features
-Commits should be as small as possible, while ensuring that each commit is correct independently (i.e., each commit should compile and pass tests).
-
-Make sure to include `Closes #<issue number>` or `Fixes #<issue number>` in the pull request description.
-
-### Pull requests with bug fixes
-Please make all changes in one commit if possible. Include `Closes #<issue number>` in bottom of the commit message.
-A commit message for a bug fix should look something like this:
-
-```
-Dashboard: Avoid infinite loop in the dashboard provisioner
-
-If one dashboard with an uid is refered to by two
-provsioners each provisioner overwrite each other.
-filling up dashboard_versions quite fast if using
-default settings.
-
-Closes #12864
-```
-
-For more details about PR naming and commit messages please see [PR and commit messages guidelines](#PR-and-commit-messages-guidelines)
+- Branch from the master branch and, if needed, rebase to the current master branch before submitting your pull request. If it doesn't merge cleanly with master you may be asked to rebase your changes.
 
-If the pull request needs changes before its merged the new commits should be rebased into one commit before its merged.
+- If your patch is not getting reviewed or you need a specific person to review it, you can @-reply a reviewer asking for a review in the pull request or a comment.
 
-## Backend dependency management
+- Add tests relevant to the fixed bug or new feature.
 
-The Grafana project uses [Go modules](https://golang.org/cmd/go/#hdr-Modules__module_versions__and_more) to manage dependencies on external packages. This requires a working Go environment with version 1.11 or greater installed.
+- Follow [PR and commit messages guidelines](#PR-and-commit-messages-guidelines)
 
-All dependencies are vendored in the `vendor/` directory.
+### Pull Requests titles and message
 
-To add or update a new dependency, use the `go get` command:
+Pull request titles should follow this format: `Area: Name of the change`.
+Titles are used to generate the changelog so they should be as descriptive as possible in one line.
 
-```bash
-# Pick the latest tagged release.
-go get example.com/some/module/pkg
+Good Examples
 
-# Pick a specific version.
-go get example.com/some/module/pkg@vX.Y.Z
-```
-
-Tidy up the `go.mod` and `go.sum` files and copy the new/updated dependency to the `vendor/` directory:
-
-
-```bash
-# The GO111MODULE variable can be omitted when the code isn't located in GOPATH.
-GO111MODULE=on go mod tidy
-
-GO111MODULE=on go mod vendor
-```
-
-You have to commit the changes to `go.mod`, `go.sum` and the `vendor/` directory before submitting the pull request.
+- `Explore: Adds Live option for supported datasources`
+- `GraphPanel: Don't sort series when legend table & sort column is not visible`
+- `Build: Support publishing MSI to grafana.com`
 
-## PR and commit messages guidelines
-PR title and squash commit messages should follow guidelines below:
+The message in the Pull requests should contain a reference so the issue if there is one. Ex `Closes #<issue number>`, `Fixes #<issue number>`, or `Ref #<issue number>` if the change is related to an issue but does not close it. Make sure to explain what problem the pull request is solving and why its implemented this way. As a new contributor its often better to overcommunicate to avoid back and forth communication, as it consumes time and energy.
 
-```
-Area of changes: Message
+### GIT commit formating.
 
-Detailed description
-```
+Grafana Squash Pull requests when merging them into master. This means the maintainer will be responsible for the title in the git commit message.
+The commit message of the commits in the Pull Request can still be part of the git commit body. So it's always encouraged to write informative commit messages.
 
-The `Area of changes` is related either to functional domain (i.e. Build, Release) or feature domain (i.e. Explore, Plugins, BarGaugePanel).
+The Git commit title should be short, descriptive and include the Pull Request ID.
 
+Good Examples
 
-`Message` should be concise, written in present tense and start with capitalised verb. Detailed description should be provided as commit message body, by entering a blank line between commit title and the description.
+- `Explore: Live supprt in datasources (#12345)`
+- `GraphPanel: Fix legend sorting issues (#12345)`
+- `Build: Support publishing MSI to grafana.com (#12345)`
 
-### Examples of good PR titles/commit messages:
-- `Explore: Adds Live option for supported datasources`
-- `GraphPanel: Don't sort series when legend table & sort column is not visible`
-- `Build: Support publishing MSI to grafana.com`
+Its also good practice to include a reference to the issue in the git commit body when possible.

pkg/ARCHITECTURE.md

@@ -0,0 +1,44 @@
+# 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 datasources 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`.
+
+## 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 [datasources](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 datasources and dashboards but this is something we want to support all over Grafana.

pkg/README.md

@@ -0,0 +1,59 @@
+# Grafana backend codebase
+
+The code [styleguide](STYLEGUIDE.md) and brief description of the [architecture](ARCHITECTURE.md)
+
+# On going refactorings.
+These issues are not something we want to address all at once but something we will improve over time. Since Grafana is released at a regular schedule the prefer approuch is to do this in batches. Not only is it easier to review, it also reduces the risk of conflicts when cherry-picking fixes from master to release branches. Changes that spawn multiple locations are therefore prefered in the end of the release cycle since we make fewer patch releases in the end of the cycle.
+
+## Global state
+Global state makes testing and debugging software harder and its something we want to avoid when possible.
+Unfortunately, there is quite a lot of global state in Grafana. The way we want to migrate away from this
+is to use 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
+
+## Reduce the use of the init() function
+Should only be used 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/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 migrated away from using Goconvey and use stdlib testing as its the most common approuch in the GO community and we think it will make it easier for new contributors. Read more about how we want to write tests in the [ARCHITECTURE.MD](/ARCHITECTURE.md#Testing) docs.
+
+## Sqlstore refactoring
+The sqlstore handlers all use a global xorm engine variable. This should be refactored to use the Sqlstore instance.
+
+## Avoid global HTTP Handler functions
+HTTP handlers should be refactored to so the handler methods are on the HttpServer instance or a more detailed handler struct. E.g (AuthHandler). This way they get access to HttpServer service dependencies (& Cfg object) and can avoid global state
+
+# Dependency management
+
+The Grafana project uses [Go modules](https://golang.org/cmd/go/#hdr-Modules__module_versions__and_more) to manage dependencies on external packages. This requires a working Go environment with version 1.11 or greater installed.
+
+All dependencies are vendored in the `vendor/` directory.
+
+To add or update a new dependency, use the `go get` command:
+
+```bash
+# Pick the latest tagged release.
+go get example.com/some/module/pkg
+
+# Pick a specific version.
+go get example.com/some/module/pkg@vX.Y.Z
+```
+
+Tidy up the `go.mod` and `go.sum` files and copy the new/updated dependency to the `vendor/` directory:
+
+```bash
+# The GO111MODULE variable can be omitted when the code isn't located in GOPATH.
+GO111MODULE=on go mod tidy
+
+GO111MODULE=on go mod vendor
+```
+
+You have to commit the changes to `go.mod`, `go.sum` and the `vendor/` directory before submitting the pull request.
+
+

style_guides/backend.md → pkg/STYLEGUIDE.md

@@ -4,30 +4,19 @@ Grafanas backend has been developed for a long time with a mix of code styles.
 
 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)
 
-
-## Global state
-Global state makes testing and debugging software harder and its something we want to avoid when possible.
-Unfortunately, there is quite a lot of global state in Grafana. The way we want to migrate away from this
-is to use 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
-
-### the `bus`
-`bus.Dispatch` is used in many places and something we want to avoid in the future since it refers to a global instance.
-The preferred solution, in this case, is to inject the `bus` into services or take the bus instance as a parameter into functions.
-
-### settings package
-In the `setting` packages there are many global variables which Grafana sets at startup. This is also something we want to move
-away from and move as much configuration as possible to the `setting.Cfg` struct and pass it around, just like the bus.
-
 ## Linting and formatting
 We enforce strict `gofmt` formating and use some linters on our codebase. You can find the current list of linters at https://github.com/grafana/grafana/blob/master/scripts/backend-lint.sh
 
 We use [revive](https://github.com/mgechev/revive) as a go linter, and do enforce our [custom config](https://github.com/grafana/grafana/blob/master/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
 
-For smaller tests its preferred to use standard library 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. 
+
+For new tests its preferred to use standard library testing.
 
 ### 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.