Add journald receiver #5160

luckyj5 · 2021-09-08T07:47:20Z

Description: Add Journald receiver
Link to tracking Issue: #2332

Testing: Unit tests are included

Documentation: Journald receiver README.md is included

djaglowski

The new module looks fine, with a few comments to address.

However, I am not clear on the implications of adding journalctl to the dockerfile. Is this best approach? Is it wise to manage such a large set of specific files in this way? Are we sure this is the right set of files? Will this be a fragile pattern? At a minimum, I think we need a test that will fail if journald cannot run due to changes in the set of files that must be copied into the image.

We also have a challenge with the windows tests failing. opentelemetry-log-collection only builds the journald operator for linux builds. I would think we want a way to include this receiver only in linux builds, but I don't think we have such a mechanism in place for the collector. Any ideas on the best approach here?

receiver/journaldreceiver/go.mod

receiver/journaldreceiver/README.md

receiver/journaldreceiver/testdata/config.yaml

luckyj5 · 2021-09-08T22:26:47Z

The new module looks fine, with a few comments to address.

However, I am not clear on the implications of adding journalctl to the dockerfile. Is this best approach? Is it wise to manage such a large set of specific files in this way? Are we sure this is the right set of files? Will this be a fragile pattern? At a minimum, I think we need a test that will fail if journald cannot run due to changes in the set of files that must be copied into the image.

We also have a challenge with the windows tests failing. opentelemetry-log-collection only builds the journald operator for linux builds. I would think we want a way to include this receiver only in linux builds, but I don't think we have such a mechanism in place for the collector. Any ideas on the best approach here?

@djaglowski Thanks. PR is updated per your comments.
Regarding best approach for adding journalctl to the dockerfile, its added based on the discussion here #2332.

We'll look into it a bit more and share our thoughts. Thanks!

gillg · 2021-09-09T08:07:10Z

I like the global idea of this receiver I was searching for it some weeks ago and I fallbacked on syslog. Moreover reuse the sideproject for logs collection seems perfect for the consistency.
But I'm a little bit affraid about the difficult to maintain the docker image with journalctl installed by this way... We could have broken builds if the debian version change, incompatibilities between amd64 / arm64 builds, cross OS difficulties and so on.

Maybe an alternative could be to propose install systemd-journal-remote to the user like we need to install rsyslog to use syslog receiver ?
In this case instead of read local files with journalctl we call an HTTP endpoint (native journald proxy) to receive the journal content in a pre-translated format (json probably). Features of the HTTP endpoint are the same than journalctl.

rockb1017 · 2021-09-10T23:40:59Z

First, I would like to know how to not include journald receiver when building it for non-linux OS because it will only work for linux. Please let me know if you have a preferred way for it.

rockb1017 · 2021-09-10T23:46:54Z

But I'm a little bit affraid about the difficult to maintain the docker image with journalctl installed by this way... We could have broken builds if the debian version change, incompatibilities between amd64 / arm64 builds, cross OS difficulties and so on.

it has been updated 6 months ago and no changes since then. I think we will be fine since we use a specific version tag 10.8 not latest.

In this case instead of read local files with journalctl we bind an HTTP endpoint to receive the journal content in a pre-translated format (json probably).
what is sending journald logs to HTTP endpoint? I am not fan of requiring any operation on users host side to make it work.

djaglowski · 2021-09-15T16:08:34Z

@rockb1017 I think we may want to do the following to the log-collection library: https://github.com/open-telemetry/opentelemetry-log-collection/pull/267/files

This should allow all operating systems to compile and pass tests, while still omitting the journald operator from non-linux builds. The result, in theory, is that users who try to use the journald receiver will get an error from the log-collection library, along the lines of "unknown operator 'journald'". (If we go this route, we may want to capture the error and wrap it in something clearer to the user.)

rockb1017 · 2021-09-15T17:49:38Z

@djaglowski Thanks for replying. I tried something along that approach but build still fails with this. receiver/journaldreceiver/journald.go:68:14: undefined: journald.NewJournaldInputConfig
it is because in internal/components/components.go, it calls journaldreceiver.NewFactory()

djaglowski · 2021-09-15T18:50:13Z

@rockb1017 I have opened an issue to discuss excluding components such as this, based on OS.

If this is not implemented, or not soon enough, I think the next best option may be to write a full "dummy" implementation for non-linux builds. Something like:

// build !linux

package journaldreceiver

import (
	"go.opentelemetry.io/collector/component"
	"go.opentelemetry.io/collector/config"
)

const typeStr = "journald"

// NewFactory creates a dummy factory.
func NewFactory() component.ReceiverFactory {
	return receiverhelper.NewFactory(
		typeStr,
		createDefaultConfig,
		receiverhelper.WithLogs(createLogsReceiver))
}

func createDefaultConfig() config.Receiver {
	return nil // or whatever dummy is necessary

func createLogsReceiver(
	_ context.Context,
	_ component.ReceiverCreateSettings,
	_ config.Receiver,
	_ consumer.Metrics,
) (component.MetricsReceiver, error) {
	return nil, fmt.Errorf("'journald' only supported on linux")
}

EDIT: This would seem to be the best current approach for "excluding" a component from a specific OS. It does not look like the builder will be changed to handle this.

gillg · 2021-09-15T22:50:07Z

Hello again.
I bring some new information with my strategy to avoid OS dependancies on OTEL collector build and docker image.
If you want expose journald in a readable format, journalctl is the way localy and journald-gateway is the way remotely.

My idea is :

We use a sidecar pattern container (called "journald-gateway" as exemple) built with something like that :

FROM debian:stable-slim

RUN apt update && apt install -y systemd-journal-remote && apt clean
# For RHEL : yum install systemd-journal-gateway

EXPOSE 19531
 
# Doc https://www.freedesktop.org/software/systemd/man/systemd-journal-gatewayd.service.html
ENTRYPOINT ["/lib/systemd/systemd-journal-gatewayd"]
# For RHEL : /usr/lib/systemd/systemd-journal-gatewayd

CMD ["--directory=/host/var/log/journal"]

We create it on host as sidecar with docker run -v /var/log/journal/:/host/var/log/journal/ -ti journald-gateway
Otel container can reach it with http://journald-gateway:19531/entries?follow and http://journald-gateway:19531/machine to build resources attributes eventualy
On otel collector we use a config like :

receiver:
  journald:
    gateway:
      endpoint: http://journald-gateway:19531/
      units:  # can be used to filter the /entries http route

# directory and files parameters can be passed to the journald-gateway container at startup but not at otel-collector level

All documentation here https://www.freedesktop.org/software/systemd/man/systemd-journal-gatewayd.service.html#Supported%20URLs
We can eventualy have only one collector for multiple nodes if we don't want the sidecar pattern.

Probably, this change a lot the actual code but not the goal, and don't disturb otel-collector.

gillg · 2021-09-15T23:01:27Z

cmd/otelcontribcol/Dockerfile

+# This is required by the podmanreceiver.
+RUN mkdir -p /rundir
+RUN mkdir -p /tmp
+RUN chown -R ${USER_UID}:${USER_UID} /rundir


As already said, work on Dockerfile and introduce kind of complexity will probably introduce issues between containers runtimes and OS.
If tomorrow we provide an image for windows container (PR already exists) as an exemple, these steps will not be compatible, while just copying otelcontribcol is.

the dockerfile for windows would be a separate file yea? so it wouldn't affect the image for windows.

Hey @gillg thanks. Just to clarify are you referring line 8-11 in the Dockerfile or changes related to journalctl?

Not 100% sure if windows will be a different file. The image can be just an ARG and buildkit can create all flavors.
And my comment is pretty general but just trigger here last changes related to adaptations made in podman context.

rockb1017 · 2021-09-15T23:09:53Z

@gillg
Thanks for looking into it and providing alternatives.
I think it makes deployment complicated. in the k8s environment, all will be wrapped in a helm chart, so it doesn't matter much but for any other use case, it makes it complicated to collect journald logs.
Also when it is consuming from gateway, how would it track offset for checkpointing? is it possible to make sure it consumes journald logs without data loss and data duplication when pod gets restarted?

gillg · 2021-09-15T23:20:53Z

Also when it is consuming from gateway, how would it track offset for checkpointing? is it possible to make sure it consumes journald logs without data loss and data duplication when pod gets restarted?

Good point, you can use Range query parameter to start to a specific offset, you can put a cursor reference in a file or shared parameter. How do you deal it with journalctl ? Parameters are pretty the same.
And about dedup yes I don't have solution, but seems similar with journalctl if you start 2 container mounted on journal folder, no ?

rockb1017 · 2021-09-16T19:12:39Z

@gillg oh there is no checkpoint mechanism with journald input operator. I assumed there is.

djaglowski · 2021-09-16T19:16:07Z

@gillg oh there is no checkpoint mechanism with journald input operator. I assumed there is.

Actually, there is. See references to cursor

rockb1017 · 2021-09-16T21:14:11Z

@djaglowski Awesome! yes it does have it. i missed that part. so we just need to enable storage extension just like filelog, yea?

djaglowski · 2021-09-16T21:21:53Z

so we just need to enable storage extension just like filelog, yea?

Yes, it should work exactly the same.

djaglowski · 2021-09-29T16:33:39Z

Please see comment here about using a stub receiver for non-linux OS.

djaglowski · 2021-09-29T16:34:38Z

Also, the main branch was broken this morning, but was just fixed. Pulling in the update should resolve many of these failures.

djaglowski · 2021-10-05T19:54:07Z

cmd/otelcontribcol/Dockerfile

 COPY --from=prep /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
 COPY otelcontribcol /
 EXPOSE 4317 55680 55679
 ENTRYPOINT ["/otelcontribcol"]
-CMD ["--config", "/etc/otel/config.yaml"]
+CMD ["--config", "/etc/otel/config.yaml"]


Please add a newline

cmd/otelcontribcol/Dockerfile

djaglowski · 2021-10-05T20:01:32Z

receiver/journaldreceiver/README.md

@@ -0,0 +1,26 @@
+## `Journald Receiver`
+
+Parses Journald events using using the [opentelemetry-log-collection](https://github.com/open-telemetry/opentelemetry-log-collection) library.


Please rephrase as a complete sentence and remove the duplicate word.

Done. Let me know if anything specific needs to be added. Thanks!

djaglowski · 2021-10-05T20:04:04Z

receiver/journaldreceiver/README.md

+
+| Field                  | Default          | Description                                                                                                        |
+| ---                    | ---              | ---                                                                                                                |
+| `directory`            | /run/log/journal | A directory containing journal files to read entries from     |


The underlying operator will by default read from either /run/log/journal or /run/journal, whichever is present. Can you please note both here?

journald_input docs

receiver/journaldreceiver/README.md

djaglowski · 2021-10-05T20:05:54Z

receiver/journaldreceiver/coverage.out

@@ -0,0 +1 @@
+mode: atomic


I think this file is supposed to be excluded?

Yes, thank you for pointing this. Removed it

receiver/journaldreceiver/journald_nonlinux.go

djaglowski · 2021-10-05T20:08:42Z

receiver/journaldreceiver/testdata/config.yaml

+    logs:
+      receivers: [journald]
+      processors: [nop]
+      exporters: [nop]


Newline please

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

djaglowski

The receiver code looks good to me, but there are two open questions in my mind:

Are the changes to the Dockerfile acceptable? Should we consider adding a dedicated Dockerfile that enables this receiver? I'm not sure the if the implementation of that approach would be reasonable either. @tigrannajaryan, do you have an opinion on this?
Can we have an integration test, and/or test(s) in the testbed? Perhaps take inspiration from the containerized integration tests in other receivers.

dmitryax · 2021-10-07T06:10:24Z

Are the changes to the Dockerfile acceptable? Should we consider adding a dedicated Dockerfile that enables this receiver? I'm not sure the if the implementation of that approach would be reasonable either. @tigrannajaryan, do you have an opinion on this?

I would suggest if we can remove dockerfile changes from this PR and make it clear in the docs that this receiver depends on journald to be available on the system.

We can handle containerized environments later -- I would lean towards having a dedicated Dockerfile. That part can probably go to https://github.com/open-telemetry/opentelemetry-collector-releases.

If this is an acceptable solution, I think we also need to add a graceful handling of journald absence, similar to how we handle Windows now.

jpkrohling · 2021-10-07T08:16:38Z

I would suggest if we can remove dockerfile changes from this PR and make it clear in the docs that this receiver depends on journald to be available on the system.

+1, I don't think journald is even recommended to exist in containers...

tigrannajaryan · 2021-10-07T10:15:02Z

I would suggest if we can remove dockerfile changes from this PR and make it clear in the docs that this receiver depends on journald to be available on the system.

+1.

@dmitryax I assume for the use case we have for journald (to be added to Helm Chart) this is not a problem? (I don't know, just want to make sure).

gillg · 2021-10-07T14:01:38Z

@tigrannajaryan @jpkrohling see my summarized proposal at the end of related issue.
Maybe we can document one or two solutions for users.

dmitryax · 2021-10-07T16:57:23Z

@dmitryax I assume for the use case we have for journald (to be added to Helm Chart) this is not a problem? (I don't know, just want to make sure).

@tigrannajaryan for now we can enhance docker image in our own distro since we do want to use journald by default

djaglowski · 2021-10-11T17:03:15Z

Incorporating the conversation above, this is my understanding of what needs to be done on this PR:

Changes to the Dockerfile should be reverted.
The receiver will, in this initial iteration, depend upon the presence of journalctl, but should error gracefully if it is not present.
A future PR can add an option to use journald-gateway, and possibly and associated Dockerfile.

luckyj5 · 2021-10-11T21:53:25Z

Incorporating the conversation above, this is my understanding of what needs to be done on this PR:

Changes to the Dockerfile should be reverted.

The receiver will, in this initial iteration, depend upon the presence of journalctl, but should error gracefully if it is not present.

A future PR can add an option to use journald-gateway, and possibly and associated Dockerfile.

Thanks @djaglowski.

I reverted the dockerfile changes.
Regarding graceful error handling, I think it should be implemented here? - https://github.com/open-telemetry/opentelemetry-log-collection/blob/a54a0a1dd0823a6758b8a240040e626a3122f8b0/operator/builtin/input/journald/journald.go#L149-L154

rockb1017 · 2021-10-11T21:58:09Z

Thanks for the review and the discussions.
Regarding graceful handling of missing journalctl, why should it be graceful error (warning)? we can let it fail the agent until user disable journald receiver input in the config (or add journalctl to host).
Anyways, like @luckyj5 said, it it should be implemented in log library, can we get this PR merged ?

luckyj5 · 2021-10-11T22:47:14Z

Update on unsuccessful checks:

Pipeline failures are not related to this PR.

Failed integration tests belongs to jmx receiver and were added by this pr
And build-and-test / loadtest (TestBallastMemory|TestLog10kDPS) test did fail on other PRs which are already merged.

djaglowski · 2021-10-12T14:37:25Z

I think graceful handling of a missing journalctl primarily means that receiver fails in an expected manner, and that it returns an error message that sufficiently conveys the cause of the failure to the user.

The current the error message appears sufficient for this purpose, though perhaps could be improved upon in the future:

Error: cannot start receivers: start stanza: start journalctl: exec: "journalctl": executable file not found in $PATH
2021/10/11 20:20:42 collector server run finished with error: cannot start receivers: start stanza: start journalctl: exec: "journalctl": executable file not found in $PATH

receiver/journaldreceiver/README.md

djaglowski

LGTM

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

…5160) * change feature registry as a prototype to make test friendly Signed-off-by: Kay Yan <kay.yan@daocloud.io> * Change colTelemetry to work with a custom Registry, improve tests Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Fix tests, windows, deprecate old API Co-authored-by: Alex Boten <aboten@lightstep.com>

luckyj5 requested a review from a team as a code owner September 8, 2021 07:47

luckyj5 requested a review from codeboten September 8, 2021 07:47

project-bot bot added this to In progress in Collector Sep 8, 2021

github-actions bot assigned djaglowski Sep 8, 2021

djaglowski reviewed Sep 8, 2021

View reviewed changes

gillg reviewed Sep 15, 2021

View reviewed changes

luckyj5 force-pushed the add-journald-receiver branch 3 times, most recently from 32d794a to c606e03 Compare September 27, 2021 22:58

luckyj5 force-pushed the add-journald-receiver branch from bcbca7c to c06d751 Compare October 1, 2021 22:05

luckyj5 requested review from alolita, Aneurysm9, anuraaga and arminru as code owners October 1, 2021 22:06

luckyj5 marked this pull request as ready for review October 5, 2021 18:11

github-actions bot assigned Aneurysm9 Oct 5, 2021

djaglowski reviewed Oct 5, 2021

View reviewed changes

luckyj5 and others added 2 commits October 5, 2021 13:15

Update receiver/journaldreceiver/README.md format

e0c8b93

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

updated docs, comments and added new line

c4a2345

djaglowski reviewed Oct 6, 2021

View reviewed changes

Merge branch 'open-telemetry:main' into add-journald-receiver

1a88d35

rockb1017 mentioned this pull request Oct 11, 2021

adding journalctl to image signalfx/splunk-otel-collector#715

Closed

luckyj5 added 2 commits October 11, 2021 14:50

Reverted dockerfile changes and updated readme

9c1801a

revert Dockerfile changes

2428fa2

djaglowski reviewed Oct 12, 2021

View reviewed changes

receiver/journaldreceiver/README.md Outdated Show resolved Hide resolved

djaglowski approved these changes Oct 12, 2021

View reviewed changes

Collector automation moved this from In progress to Reviewer approved Oct 12, 2021

Update receiver/journaldreceiver/README.md

3ff3491

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

djaglowski added the ready to merge Code review completed; ready to merge by maintainers label Oct 12, 2021

tigrannajaryan approved these changes Oct 13, 2021

View reviewed changes

tigrannajaryan merged commit fab94ea into open-telemetry:main Oct 13, 2021

Collector automation moved this from Reviewer approved to Done Oct 13, 2021

tigrannajaryan mentioned this pull request Oct 13, 2021

Add journald receiver #2332

Closed

		@@ -0,0 +1,26 @@
		## `Journald Receiver`

		Parses Journald events using using the [opentelemetry-log-collection](https://github.com/open-telemetry/opentelemetry-log-collection) library.

Add journald receiver #5160

Add journald receiver #5160

Conversation

luckyj5 commented Sep 8, 2021

djaglowski left a comment

Choose a reason for hiding this comment

luckyj5 commented Sep 8, 2021

gillg commented Sep 9, 2021 • edited Loading

rockb1017 commented Sep 10, 2021

rockb1017 commented Sep 10, 2021 • edited Loading

djaglowski commented Sep 15, 2021

rockb1017 commented Sep 15, 2021

djaglowski commented Sep 15, 2021 • edited Loading

gillg commented Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

rockb1017 Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockb1017 commented Sep 15, 2021

gillg commented Sep 15, 2021

rockb1017 commented Sep 16, 2021

djaglowski commented Sep 16, 2021

rockb1017 commented Sep 16, 2021

djaglowski commented Sep 16, 2021

djaglowski commented Sep 29, 2021

djaglowski commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

dmitryax commented Oct 7, 2021 • edited Loading

jpkrohling commented Oct 7, 2021

tigrannajaryan commented Oct 7, 2021

gillg commented Oct 7, 2021

dmitryax commented Oct 7, 2021

djaglowski commented Oct 11, 2021

luckyj5 commented Oct 11, 2021 • edited Loading

rockb1017 commented Oct 11, 2021 • edited Loading

luckyj5 commented Oct 11, 2021 • edited Loading

djaglowski commented Oct 12, 2021

djaglowski left a comment

Choose a reason for hiding this comment

gillg commented Sep 9, 2021 •

edited

Loading

rockb1017 commented Sep 10, 2021 •

edited

Loading

djaglowski commented Sep 15, 2021 •

edited

Loading

gillg commented Sep 15, 2021 •

edited

Loading

rockb1017 Sep 15, 2021 •

edited

Loading

dmitryax commented Oct 7, 2021 •

edited

Loading

luckyj5 commented Oct 11, 2021 •

edited

Loading

rockb1017 commented Oct 11, 2021 •

edited

Loading

luckyj5 commented Oct 11, 2021 •

edited

Loading