fluent-plugin-loki: Restructuring and CI #2006

Skeen · 2020-04-28T15:03:10Z

What this PR does / why we need it:
This PR:

Moves fluentd/fluent-plugin-grafana-loki to cmd/fluentd, as to follow the standard for the other integrations.
Integrates it with the Makefile, such that the commands fluentd-plugin, fluentd-image, fluentd-push and fluentd-test are added, as they are for the fluent-bit plugin.
Adds a drone-ci job to generate and keep the FluentD images up to date.

Which issue(s) this PR fixes:
Fixes #1724.

Special notes for your reviewer:
I have removed the dependency on systemd, as it seems to have only been needed for the specific configuration in systemd.conf, assuming a user needs this, they will need to create a derivative image with this as base image.

Checklist

Documentation added
Tests updated

cyriltovena · 2020-04-28T23:19:44Z

Wow we really needed that !! I’ll take a look this week, thank you !

adityacs · 2020-04-29T05:35:28Z

@Skeen Thanks for this PR.

@cyriltovena I think we should also include publishing the fluentd gem to gem repository in this PR and also we can move both fluentbit and fluentd under plugins folder as discussed here #1989 WDYT?

Skeen · 2020-04-29T08:51:14Z

@adityacs as for publishing the Flutentd gem, I'm alfraid that I won't be able to help with that. I don't have any experience with Ruby.

Also, do we want to move them right away, or would we rather do it using two PRs:

This PR, which unifies the structure
A new PR, which restructures and moves the plugins to a seperate folder.

I'd personally argue for two seperate PRs, mainly to keep changes simple and traceable.

cyriltovena · 2020-04-29T13:36:45Z

It's ok for the gem, I can take over.

Yeah @adityacs this is a good idea, but let's send another PR for it.

adityacs · 2020-04-29T14:01:41Z

@cyriltovena Sure

.drone/drone.jsonnet

Makefile

cmd/fluentd/Dockerfile

cyriltovena

I'm wondering if the default conf we are providing in the container is a good one.

Good job on the PR. I'm happy to merge it as is although I'd like to see what you think could be a better default config for fluentd.

Alternatively we can update the doc in https://github.com/grafana/loki/tree/master/docs/clients/fluentd#docker-image it's a bit out of date and could use a configuration example.

No idea why but on the second run of make fluentd-image I'm getting

bundle install --gemfile=cmd/fluentd/Gemfile --path=cmd/fluentd/vendor/bundle
You must use Bundler 2 or greater with this lockfile.
make: *** [Makefile:425: fluentd-plugin] Error 20
The command '/bin/sh -c make BUILD_IN_CONTAINER=false fluentd-plugin' returned a non-zero code: 2
make: *** [fluentd-image] Error 2

PS: would be nice to update the image in the doc too.

Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Skeen · 2020-04-30T12:25:25Z

I'm wondering if the default conf we are providing in the container is a good one.

Providing default configuration files are a difficult task as usage patterns are so diverse.

... I'd like to see what you think could be a better default config for fluentd.

If it was purely up to me, i think I'd just go with the default configuration that ships with FluentD, appended with the @include loki.conf line.

The argument for this approach would be, that we do not in fact take a stand with regards to the default configuration, but rather outsource this decision to upstream, and as such, if people are displased with the default configuration, it is not our issue, but rather an issue for them to take with upstream.

The default configuration for upstream looks like this (as of fluent/fluentd:v1.9.2-debian-1.0):

<source>
  @type  forward
  @id    input1
  @label @mainstream
  port  24224
</source>

<filter **>
  @type stdout
</filter>

<label @mainstream>
  <match docker.**>
    @type file
    @id   output_docker1
    path         /fluentd/log/docker.*.log
    symlink_path /fluentd/log/docker.log
    append       true
    time_slice_format %Y%m%d
    time_slice_wait   1m
    time_format       %Y%m%dT%H%M%S%z
  </match>
  <match **>
    @type file
    @id   output1
    path         /fluentd/log/data.*.log
    symlink_path /fluentd/log/data.log
    append       true
    time_slice_format %Y%m%d
    time_slice_wait   10m
    time_format       %Y%m%dT%H%M%S%z
  </match>
</label>

Alternatively we can update the doc in https://github.com/grafana/loki/tree/master/docs/clients/fluentd#docker-image it's a bit out of date and could use a configuration example.

Assuming we switched to using upstreams default configuration file, we may also be able to refer people to upstreams documentation, and purely document the loki.confand the configuration variables defined within this file.

However changing the default configuration file in general (which is already done in this PR, by eliminating the systemd dependency), has the issue of breaking backwards compatability for users running with the default configuration file. I don't know if this is a big issue, but it could be.

An alternative solution to breaking backwards compatability, would be to deprecate the old docker registry, and create a new one with this change. - But I don't know how likely it is that users are actually using the default configuration in production environments.

Skeen · 2020-04-30T12:32:56Z

No idea why but on the second run of make fluentd-image I'm getting

bundle install --gemfile=cmd/fluentd/Gemfile --path=cmd/fluentd/vendor/bundle
You must use Bundler 2 or greater with this lockfile.
make: *** [Makefile:425: fluentd-plugin] Error 20
The command '/bin/sh -c make BUILD_IN_CONTAINER=false fluentd-plugin' returned a non-zero code: 2
make: *** [fluentd-image] Error 2

I'm afraid I have little to no experience with Ruby or it's toolchains, but I imagine a very crude workaround would be to simply remove the lockfile if it exists before trying to bundle install ...

cyriltovena · 2020-04-30T15:34:31Z

Could not find a lock file, I'll figure this out.

However changing the default configuration file in general (which is already done in this PR, by eliminating the systemd dependency), has the issue of breaking backwards compatability for users running with the default configuration file. I don't know if this is a big issue, but it could be.

Nobody is really using that image since it's wasn't pushed first, I'm not worried that you go with the suggestion you have, I like it.

…ta-aps/loki into feature/docker-image-fluentd-amd64

codecov-io · 2020-05-01T12:54:30Z

Codecov Report

Merging #2006 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2006      +/-   ##
==========================================
- Coverage   63.83%   63.76%   -0.07%     
==========================================
  Files         132      132              
  Lines       10150    10150              
==========================================
- Hits         6479     6472       -7     
- Misses       3189     3193       +4     
- Partials      482      485       +3

Impacted Files	Coverage Δ
pkg/promtail/targets/tailer.go	`73.86% <0.00%> (-4.55%)`	⬇️
pkg/promtail/targets/filetarget.go	`68.29% <0.00%> (-1.83%)`	⬇️

Skeen · 2020-05-01T12:58:31Z

@cyriltovena Am I right in assuming, that you do not expect me to make other changes before this is mergeable, or did I miss something?

.drone/drone.yml

cmd/fluentd/docker/docker-compose.yml

cyriltovena · 2020-05-01T13:08:24Z

cmd/fluentd/docker/conf/loki.conf

+<match loki.**>
+  @type loki
+  url "#{ENV['LOKI_URL']}"
+  extra_labels {"env":"dev"}


maybe job="fluentd" is a better default ?

Done in 244f196

cyriltovena · 2020-05-01T13:09:46Z

@cyriltovena Am I right in assuming, that you do not expect me to make other changes before this is mergeable, or did I miss something?

No just have a look at the question I have.

cyriltovena

LGTM

cyriltovena · 2020-05-01T16:13:14Z

If you're up for a bit of documentation improvement that would greatly help again Thank you for this !

Skeen added 3 commits April 28, 2020 15:28

Move fluentd/fluent-plugin-grafana-loki to cmd/fluentd

55f196f

Integrate fluentd with Makefile

c4af2ad

Added drone-ci job for fluentd

243241f

cyriltovena reviewed Apr 29, 2020

View reviewed changes

.drone/drone.jsonnet Outdated Show resolved Hide resolved

cyriltovena reviewed Apr 29, 2020

View reviewed changes

Makefile Show resolved Hide resolved

cyriltovena reviewed Apr 29, 2020

View reviewed changes

cmd/fluentd/Dockerfile Outdated Show resolved Hide resolved

cyriltovena reviewed Apr 29, 2020

View reviewed changes

Skeen and others added 2 commits April 30, 2020 14:17

Fix typo fluent-bit --> fluentd in .drone/drone.jsonnet

5cb8c2a

Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Change default LOKI_URL in FluentD Dockerfile

8590992

Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Skeen added 3 commits May 1, 2020 12:32

Utilize upstream configuration file

bd7edb6

Merge branch 'feature/docker-image-fluentd-amd64' of github.com:magen…

610b6ad

…ta-aps/loki into feature/docker-image-fluentd-amd64

Test setup, adapt default configuration

872ae96

cyriltovena reviewed May 1, 2020

View reviewed changes

.drone/drone.yml Outdated Show resolved Hide resolved

cyriltovena reviewed May 1, 2020

View reviewed changes

cmd/fluentd/docker/docker-compose.yml Show resolved Hide resolved

cyriltovena reviewed May 1, 2020

View reviewed changes

Skeen added 2 commits May 1, 2020 15:41

Regen drone.yml

a1b54cb

Change default label to job=fluentd, rather than env=dev

244f196

cyriltovena approved these changes May 1, 2020

View reviewed changes

cyriltovena merged commit 33a8152 into grafana:master May 1, 2020

Skeen deleted the feature/docker-image-fluentd-amd64 branch May 2, 2020 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fluent-plugin-loki: Restructuring and CI #2006

fluent-plugin-loki: Restructuring and CI #2006

Skeen commented Apr 28, 2020

cyriltovena commented Apr 28, 2020

adityacs commented Apr 29, 2020 •

edited

Skeen commented Apr 29, 2020

cyriltovena commented Apr 29, 2020

adityacs commented Apr 29, 2020

cyriltovena left a comment •

edited

Skeen commented Apr 30, 2020 •

edited

Skeen commented Apr 30, 2020 •

edited

cyriltovena commented Apr 30, 2020

codecov-io commented May 1, 2020 •

edited

Skeen commented May 1, 2020 •

edited

cyriltovena May 1, 2020

Skeen May 1, 2020

cyriltovena commented May 1, 2020

cyriltovena left a comment

cyriltovena commented May 1, 2020

fluent-plugin-loki: Restructuring and CI #2006

fluent-plugin-loki: Restructuring and CI #2006

Conversation

Skeen commented Apr 28, 2020

cyriltovena commented Apr 28, 2020

adityacs commented Apr 29, 2020 • edited

Skeen commented Apr 29, 2020

cyriltovena commented Apr 29, 2020

adityacs commented Apr 29, 2020

cyriltovena left a comment • edited

Choose a reason for hiding this comment

Skeen commented Apr 30, 2020 • edited

Skeen commented Apr 30, 2020 • edited

cyriltovena commented Apr 30, 2020

codecov-io commented May 1, 2020 • edited

Codecov Report

Skeen commented May 1, 2020 • edited

cyriltovena May 1, 2020

Choose a reason for hiding this comment

Skeen May 1, 2020

Choose a reason for hiding this comment

cyriltovena commented May 1, 2020

cyriltovena left a comment

Choose a reason for hiding this comment

cyriltovena commented May 1, 2020

adityacs commented Apr 29, 2020 •

edited

cyriltovena left a comment •

edited

Skeen commented Apr 30, 2020 •

edited

Skeen commented Apr 30, 2020 •

edited

codecov-io commented May 1, 2020 •

edited

Skeen commented May 1, 2020 •

edited