Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fluent-bit shared object go plugin #847

Merged

Conversation

cosmo0920
Copy link
Contributor

@cosmo0920 cosmo0920 commented Aug 5, 2019

What this PR does / why we need it:

The first step to merge https://github.com/cosmo0920/fluent-bit-go-loki.

Which issue(s) this PR fixes:
No fixes. This is the first task of #770.

Special notes for your reviewer:

I think that we would have to create fluent-bit/fluent-bit-go-loki folder instead of cmd/fluent-bit.
Because fluent-bit-go-loki Golang plugin uses shared object format.
It is not executable.

Building steps for fluent-bit-go-loki

fluent-bit-go-loki can build with:

$ (cd fluent-bit/fluent-bit-go-loki && go build -buildmode=c-shared -o out_loki.so .)

Or, make with added Makefile(building and testing):

$ (cd fluent-bit/fluent-bit-go-loki && make)

Testing for fluent-bit-go-loki

fluent-bit-go-loki test can execute with:

$ (cd fluent-bit/fluent-bit-go-loki && go test -cover -race -coverprofile=coverage.txt -covermode=atomic)

Or, make with added Makefile:

$ (cd fluent-bit/fluent-bit-go-loki && make test)

Checklist

  • Documentation added
  • Tests updated

@cosmo0920 cosmo0920 changed the title Fluent bit shared object go plugin fluent-bit shared object go plugin Aug 5, 2019
@cosmo0920 cosmo0920 force-pushed the fluent-bit-shared-object-go-plugin branch 2 times, most recently from 654aee1 to fba2cfb Compare August 5, 2019 04:21
Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, looks pretty good so far! Just some minor nits 😄

fluent-bit/fluent-bit-go-loki/Makefile Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/README.md Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/loki.go Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/out_loki.go Outdated Show resolved Hide resolved
@sh0rez
Copy link
Member

sh0rez commented Aug 5, 2019

Regarding the placement of the fluentd / fluent-bit plugins ... Couldn't we place them in a common fluent folder? Something like

.
├── fluent
│   ├── bit
│   └── d

/cc @cyriltovena @slim-bean

@cosmo0920 cosmo0920 force-pushed the fluent-bit-shared-object-go-plugin branch from ac5f16f to ad4134f Compare August 5, 2019 13:59
Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks a lot 🎉

LGTM

@cyriltovena
Copy link
Contributor

Can you confirm the new code in fluent-bit folder is linted by the linter ?

@cosmo0920
Copy link
Contributor Author

golangci-lint complains the following warning:

% GOGC=20 golangci-lint run
WARN [runner/golint] Golint: can't lint 5 files: no file name for file &{Doc:<nil> Package:21595946 Name:main Decls:[0xc0287bd040 0xc0287bd180 0xc0287bd2c0 0xc0287bfb60] Scope:scope 0xc028786dc0 {
	func getLokiConfig
	type lokiConfig
	type labelSetJSON
}
 Imports:[0xc0287bf1a0 0xc0287bf1d0 0xc0287bf200 0xc0287bf230 0xc0287bf260 0xc0287bf290] Unresolved:[flagext time int model string string string string string string error flagext nil nil fmt strconv nil time time strconv nil json byte nil nil fmt make model model model nil] Comments:[]} 

But I have no idea what it means. 😓

@sh0rez
Copy link
Member

sh0rez commented Aug 5, 2019

Can you confirm the new code in fluent-bit folder is linted by the linter ?

It is

loki/Makefile

Lines 191 to 192 in a22be17

lint:
GOGC=20 golangci-lint run

This lints everything it can find

@cyriltovena
Copy link
Contributor

It should be the import issue passed and I think there is more but the linter seems to be skipping.

Let me have quick look.

@cyriltovena
Copy link
Contributor

You can remove the license file we have the same.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not able to run the linter, Looking into a solution in the meantime there is still some issues I'd like you to take a look.

fluent-bit/fluent-bit-go-loki/README.md Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/README.md Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/README.md Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/out_loki.go Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/out_loki.go Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/out_loki.go Outdated Show resolved Hide resolved
fluent-bit/fluent-bit-go-loki/version.go Outdated Show resolved Hide resolved
@cosmo0920 cosmo0920 force-pushed the fluent-bit-shared-object-go-plugin branch 4 times, most recently from 3bfb3e7 to 6105916 Compare August 9, 2019 08:51
@cyriltovena
Copy link
Contributor

@cosmo0920 I'm on vacation for 2 weeks, will review asap when I'm back.

@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Aug 12, 2019

Yep, thank you for notifying it.
I'm also on vacation for a week and a half.

@cosmo0920 cosmo0920 force-pushed the fluent-bit-shared-object-go-plugin branch 2 times, most recently from 1d3bb34 to b3e1e64 Compare August 19, 2019 02:19
@cyriltovena
Copy link
Contributor

I'm taking another look !

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome just a last effort on the config to match fluent. This would ease transition between the two.

@JensErat
Copy link
Contributor

When putting together a proof of concept towards Loki, we also had to extend the fluent-bit output plugin due to some issues. I will definitely extract and provide a new PR for some useful parts as soon as this PR is merged (especially added quite some testing code because debugging edge cases is a hassle with CGO shared objects).

Mostly, our issues boil down to fluent-bit records not necessarily being flat, but can be a tree-like structure. For example, the fluent-bit Kubernetes filter plugin will add meta information from Kubernetes (pod, namespace, ...) within a kubernetes "folder", and if you enable JSON parsing, whole nested JSON trees can be passed as record (think of Kubernetes audit logs that sometimes contain three or four times nested records).

This resulted in two issues for us:

  • Extracting stream labels requires a non-trivial configuration. We went for JSON like this:

    {
      "TENANT": "tenant",
      "HOSTNAME": "hostname",
      "kubernetes": {
        "container_name": "container",
        "pod_name": "pod",
        "namespace_name": "namespace"
      },
      "BOOT_ID": "_drop",
      "CONTAINER_ID": "_drop",
      "CONTAINER_ID_FULL": "_drop",
      "CONTAINER_TAG": "_drop",
      "SYSTEMD_INVOCATION_ID": "_drop",
      "SELINUX_CONTEXT": "_drop",
      "CONTAINER_NAME": "_drop",
      "MACHINE_ID": "_drop"
    }

    The semantics of this is the key being a match on the fluent-bit record label, and the value either a nested sub-match (container_name from kubernetes dictionary), the target label (which is flat in Loki) or a specific drop annotation. This is easily implementable using a recursive function (we have partially test-backed prototype code). This is pretty much a tree-aware alternative to the fluentd-plugin remove_keys and label_keys configuration. The example above keeps TENANT and HOSTNAME in their lower-case variants, and renames kubernetes.pod_name etc. to simple pod labels (we see them in queries and write them in Grafana all the time, so rather keep them short).

    Surely this is also implementable as a more simple data structure, but going for a well-defined JSON-based configuration resolved lots of issues around escaping of special characters.

  • The []byte/base64 conversion has also to be applied to keys and values beyond the outermost record layer. We wondered about lots of base64-encoded text until we realized this is based on nested labels.

I'm currently doing some minor cleanups and will push the code as-is to a fork. We would be glad if we can find some agreements on how to handle nested labels with both of the issues they bring -- and we're ready to provide code to get them running, we just have to align on how to do it properly.

@cyriltovena
Copy link
Contributor

out of curiosity do you think the same issue also happen in fluentd plugin ? For promtail we have pipeline stage to parse json however we don't yet support drop.

This looks like a nice solution but we need to make sure we use the same config across the board unless there is a good argument against.

/cc @slim-bean @rfratto WDYT ?

@cyriltovena cyriltovena force-pushed the fluent-bit-shared-object-go-plugin branch from 70ab469 to eae2560 Compare September 26, 2019 20:53
@cyriltovena cyriltovena merged commit 0cfa33d into grafana:master Sep 26, 2019
@cosmo0920 cosmo0920 deleted the fluent-bit-shared-object-go-plugin branch September 27, 2019 00:07
@JensErat
Copy link
Contributor

JensErat commented Oct 1, 2019

Thank you for the great work -- I didn't get round checking the code before now again. I really like the solution with separate relabel- and drop configuration though. At first I tried rebasing my changes against the current HEAD (as I somehow was not aware you reimplemented the small recursive function already) -- pretty much puzzled my because you came up with nearly 100% identical code but on the "wrong side" of the diffs until I realized what was going on... :)

I ported some of my edge case tests I wrote while drafting my code:

#1096

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants