# Reviews

Pull-requests require two approvals before being merged. Expect several rounds
of back and forth on reviews, non-trivial changes are rarely accepted on the
first pass. It might take some time until you see a first review so please be
patient.

All pull requests should follow the style and best practices in the
[CONTRIBUTING.md](https://github.com/influxdata/telegraf/blob/master/CONTRIBUTING.md)
document.

## Process

The review process is roughly structured as follows:

1. Submit a pull request.
   Please check that you signed the [CLA](https://www.influxdata.com/legal/cla/)
   (and [Corporate CLA](https://www.influxdata.com/legal/ccla/) if you are
   contributing code on as an employee of your company). Provide a short
   description of your submission and reference issues that you potentially
   close. Make sure the CI tests are all green and there are no linter-issues.
1. Get feedback from a first reviewer and a `ready for final review` tag.
   Please constructively work with the reviewer to get your code into a
   mergeable state (see also [below](#reviewing-plugin-code)).
1. Get a final review by one of the InfluxData maintainers. Please fix any issue
   raised.
1. Wait for the pull-request to be merged. It might take some time until your PR
   gets merged, depending on the release cycle and the type of your pull-request
   (bugfix, enhancement of existing code, new plugin, etc). Remember, it might
   be necessary to rebase your code before merge to resolve conflicts.

Please read the review comments carefully, fix the related part of the code
and/or respond in case there is anything unclear. Maintainers will add the
`waiting for response` tag to PRs to make it clear we are waiting on the
submitter for updates.
__Once the tag is added, if there is no activity on a pull request or the
contributor does not respond, our bot will automatically close the PR after two
weeks!__ If you expect a longer period of inactivity or you want to abandon a
pull request, please let us know.

In case you still want to continue with the PR, feel free to reopen it.

## Reviewing Plugin Code

- Avoid variables scoped to the package. Everything should be scoped to the
  plugin struct, since multiple instances of the same plugin are allowed and
  package-level variables will cause race conditions.
- SampleConfig must match the readme, but not include the plugin name.
- structs should include toml tags for fields that are expected to be editable
  from the config. eg `toml:"command"` (snake_case)
- plugins that want to log should declare the Telegraf logger, not use the log
  package. eg:

```Go
  Log telegraf.Logger `toml:"-"`
```

(in tests, you can do `myPlugin.Log = testutil.Logger{}`)

- Initialization and config checking should be done on the `Init() error`
  function, not in the Connect, Gather, or Start functions.
- `Init() error` should not contain connections to external services. If
  anything fails in Init, Telegraf will consider it a configuration error and
  refuse to start.
- plugins should avoid synchronization code if they are not starting goroutines.
  Plugin functions are never called in parallel.
- avoid goroutines when you don't need them and removing them would simplify the
  code
- errors should almost always be checked.
- avoid boolean fields when a string or enumerated type would be better for
  future extension. Lots of boolean fields also make the code difficult to
  maintain.
- use config.Duration instead of internal.Duration
- compose tls.ClientConfig as opposed to specifying all the TLS fields manually
- http.Client should be declared once on `Init() error` and reused, (or better
  yet, on the package if there's no client-specific configuration).
  `http.Client` has built-in concurrency protection and reuses connections
  transparently when possible.
- avoid doing network calls in loops where possible, as this has a large
  performance cost. This isn't always possible to avoid.
- when processing batches of records with multiple network requests (some
  outputs that need to partition writes do this), return an error when you want
  the whole batch to be retried, log the error when you want the batch to
  continue without the record
- consider using the StreamingProcessor interface instead of the (legacy)
  Processor interface
- avoid network calls in processors when at all possible. If it's necessary,
  it's possible, but complicated (see processor.reversedns).
- avoid dependencies when:
  - they require cgo
  - they pull in massive projects instead of small libraries
  - they could be replaced by a simple http call
  - they seem unnecessary, superfluous, or gratuitous
- consider adding build tags if plugins have OS-specific considerations
- use the right logger log levels so that Telegraf is normally quiet eg
 `plugin.Log.Debugf()` only shows up when running Telegraf with `--debug`
- consistent field types: dynamically setting the type of a field should be
  strongly avoided as it causes problems that are difficult to solve later,
  made worse by having to worry about backwards compatibility in future changes.
  For example, if an numeric value comes from a string field and it is not clear
  if the field can sometimes be a float, the author should pick either a float
  or an int, and parse that field consistently every time. Better to sometimes
  truncate a float, or to always store ints as floats, rather than changing the
  field type, which causes downstream problems with output databases.
- backwards compatibility: We work hard not to break existing configurations
  during new changes. Upgrading Telegraf should be a seamless transition.
  Possible tools to make this transition smooth are:
  - enumerable type fields that allow you to customize behavior
    (avoid boolean feature flags)
  - version fields that can be used to opt in to newer changed behavior without
    breaking old (see inputs.mysql for example)
  - a new version of the plugin if it has changed significantly
    (e.g. `outputs.influxdb` and `outputs.influxdb_v2`)
  - Logger and README deprecation warnings
  - changing the default value of a field can be okay, but will affect users who
    have not specified the field and should be approached cautiously.
  - The general rule here is "don't surprise me": users should not be caught
    off-guard by unexpected or breaking changes.

## Linting

Each pull request will have the appropriate linters checking the files for any
common mistakes. The github action Super Linter is used:
[super-linter](https://github.com/github/super-linter). If it is failing you can
click on the action and read the logs to figure out the issue. You can also run
the github action locally by following these instructions:
[run-linter-locally.md](https://github.com/github/super-linter/blob/main/docs/run-linter-locally.md).
You can find more information on each of the linters in the super linter readme.

## Testing

Sufficient unit tests must be created.  New plugins must always contain
some unit tests.  Bug fixes and enhancements should include new tests, but
they can be allowed if the reviewer thinks it would not be worth the effort.

[Table Driven Tests](https://github.com/golang/go/wiki/TableDrivenTests) are
encouraged to reduce boiler plate in unit tests.

The [stretchr/testify](https://github.com/stretchr/testify) library should be
used for assertions within the tests when possible, with preference towards
github.com/stretchr/testify/require.

Primarily use the require package to avoid cascading errors:

```go
assert.Equal(t, lhs, rhs) # avoid
require.Equal(t, lhs, rhs) # good
```

## Configuration

The config file is the primary interface and should be carefully scrutinized.

Ensure the [[SampleConfig]] and
[README](https://github.com/influxdata/telegraf/blob/master/plugins/inputs/EXAMPLE_README.md)
match with the current standards.

READMEs should:

- be spaces, not tabs
- be indented consistently, matching other READMEs
- have two `#` for comments
- have one `#` for defaults, which should always match the default value of the plugin
- include all appropriate types as a list for enumerable field types
- include a useful example, avoiding "example", "test", etc.
- include tips for any common problems
- include example output from the plugin, if input/processor/aggregator/parser/serializer

## Metric Schema

Telegraf metrics are heavily based on InfluxDB points, but have some
extensions to support other outputs and metadata.

New metrics must follow the recommended
[schema design](https://docs.influxdata.com/influxdb/latest/concepts/schema_and_data_layout/).
Each metric should be evaluated for _series cardinality_, proper use of tags vs
fields, and should use existing patterns for encoding metrics.

Metrics use `snake_case` naming style.

### Enumerations

Generally enumeration data should be encoded as a tag.  In some cases it may
be desirable to also include the data as an integer field:

```shell
net_response,result=success result_code=0i
```

### Histograms

Use tags for each range with the `le` tag, and `+Inf` for the values out of
range.  This format is inspired by the Prometheus project:

```shell
cpu,le=0.0 usage_idle_bucket=0i 1486998330000000000
cpu,le=50.0 usage_idle_bucket=2i 1486998330000000000
cpu,le=100.0 usage_idle_bucket=2i 1486998330000000000
cpu,le=+Inf usage_idle_bucket=2i 1486998330000000000
```

### Lists

Lists are tricky, but the general technique is to encode using a tag, creating
one series be item in the list.

### Counters

Counters retrieved from other projects often are in one of two styles,
monotonically increasing without reset and reset on each interval.  No attempt
should be made to switch between these two styles but if given the option it
is preferred to use the non-resetting variant.  This style is more resilient in
the face of downtime and does not contain a fixed time element.

### Source tag

When metrics are gathered from another host, the metric schema should have a tag
named "source" that contains the other host's name. See [this feature
request](https://github.com/influxdata/telegraf/issues/4413) for details.

The metric schema doesn't need to have a tag for the host running
telegraf. Telegraf agent code can add a tag named "host" and by default
containing the hostname reported by the kernel. This can be configured through
the "hostname" and "omit_hostname" agent settings.

## Go Best Practices

In general code should follow best practice describe in [Code Review
Comments](https://github.com/golang/go/wiki/CodeReviewComments).

### Networking

All network operations should have appropriate timeouts.  The ability to
cancel the option, preferably using a context, is desirable but not always
worth the implementation complexity.

### Channels

Channels should be used in judiciously as they often complicate the design and
can easily be used improperly.  Only use them when they are needed.
