-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[receiver/promtailreceiver] Add promtail receiver #14632
[receiver/promtailreceiver] Add promtail receiver #14632
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. I have one big ask regarding where the code resides, and otherwise just a few minor things.
e9f0627
to
a65e37d
Compare
53e9639
to
2d2ca70
Compare
5ce866a
to
753fa72
Compare
looking into failed checks.. |
2b170a8
to
438c2cd
Compare
@djaglowski could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I had a few questions but looks good.
.github/workflows/build-and-test.yml
Outdated
@@ -75,6 +75,9 @@ jobs: | |||
runs-on: ubuntu-latest | |||
needs: [setup-environment] | |||
steps: | |||
- run: sudo apt-get -qq update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment stating that this is required because of the promtail receiver component? I read your comment that this is required for compiling this component. In that case, would the whole build fail if this component isn't present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By promtail receiver component isn't present you mean the repo doesn't have it in the receiver directory as well as all its mentions in other places like internal/components.go
, internal/components_test.go
, versions.yaml
. Basically how it is now in the main branch, right?
If so the build will pass with or without installing libsystemd-dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could isolate the code that depends on libsystemd-dev on a specific source, which is used only when a specific build flag is passed. The makefile could then check whether this lib exists and set the appropriate build flag. For our CI, we should always enable this, but I wouldn't require people on Mac or Windows systems to have this library to build contrib locally.
This brings another question: would the promtail receiver work on Windows and Darwin builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promtail receiver will work on Windows and Darwin builds because journal target is built only for linux and when cgo is enabled: https://github.com/grafana/loki/blob/488e64e3ff1bbce70d856c3daed150b061c5614d/clients/pkg/promtail/targets/journal/journaltarget.go#L1-L2
journal target will be ignored by non-linux OS and the collector build will be successful
We could isolate the code that depends on libsystemd-dev on a specific source, which is used only when a specific build flag is passed. The makefile could then check whether this lib exists and set the appropriate build flag.
promtail depends on libsystemd-dev, and promtail receiver depends on promtail. Could you please reveal the idea in more detail? What do you mean by a specific flag? Do you mean to have a custom build flag like linux-with-libsystemd-dev
? To make it work I will need to add comment //go:build linux-with-libsystemd-dev
to all promtail receiver files, right? And to run build we will pass flag go build -tags linux-with-libsystemd-dev
That will work. Or if Loki team is agree we can add specfic flag linux-with-libsystemd-dev
to https://github.com/grafana/loki/blob/main/clients/pkg/promtail/targets/journal/journaltarget.go to isolate that code and exclude it from compilation in case linux without libsystemd-dev. It will allow exclude exactly journald part from promtail without excluding the whole promtail receiver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my last comment could be ignored
|
||
replace github.com/hpcloud/tail => github.com/grafana/tail v0.0.0-20220426200921-98e8eb28ea4c | ||
|
||
// using fork for support LogpullReceived and LogpullFields for loki - see https://github.com/cloudflare/cloudflare-go/pull/743 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the Loki team does not need this PR anymore. Can you check with them what the status is on this? The replace statement is a good temporary solution, but if that PR isn't being merged, this isn't temporary anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Loki team needs the support of LogpullReceived and LogpullFields for Loki right now. They are going to switch to Logpush in the future, but currently, this replace
is needed for Loki
replace github.com/cloudflare/cloudflare-go => github.com/cyriltovena/cloudflare-go v0.27.1-0.20211118103540-ff77400bcb93 | ||
|
||
// using fork for custom dialer support - see https://github.com/bradfitz/gomemcache/pull/86 | ||
replace github.com/bradfitz/gomemcache => github.com/grafana/gomemcache v0.0.0-20220812141859-1e3ae89e91a7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this library abandoned? No commits since January, no commits in 2021 or 2020, and I see that you haven't received any answers on your PR yet :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the library looks dead :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I received answers and did fixes. Waiting for the second round of review. Will keep an eye on this
return levelVal, true | ||
} | ||
|
||
//revive:disable:error-return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this comment, golangci-lint throws an error:
error-return: error should be the last type when returning multiple items (revive)
This code isn't mine actually, I copied it from prometheusreceiver/internal/logger.go and didn't refactor it.
I was thinking to move this code somewhere outside the components and import to promtail and prometheus receivers, but not sure if it is a good approach.
If I move the logger somewhere (to internal directory maybe?) it will be there even if promtail and prometheus components are not present in the custom setup of collector contrib, so it will be an extra code user have to have that is not used anywhere
438c2cd
to
3e35deb
Compare
3e35deb
to
0bd74f0
Compare
…promtail receiver go files, removed one replace directive from go mod
…n_perf_counters/performance_query_test.go
5ab069a
to
54e7181
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @mar4uk!
Promtail receiver brought replace directives that were needed for loki #14632 This PR removes 2 of them. Two other replaces will be removed in the next PR
Description:
Added new promtail receiver component
Link to tracking Issue:
#9800
Documentation:
Currently, this receiver supports scrape_config static_configs and scrape_configs loki_push_api. I didn't check other scrape_config targets (syslog, kubernetes, window_events), some additional fixes are probably needed to make them work
The config file will look like this:
To send logs to loki via collector we can run curl:
To send logs to loki via collector we tail file example.log. It could be tested with command:
The initial draft PR was closed because of uncertainty with the license. According to https://github.com/grafana/loki/blob/main/LICENSING.md#apache-20
clients/
is under Apache-2.0 license and we can use its code in collector-contribImportant
PR grafana/loki#7320 should be merged first, it contains mapstructure support in promtail