Go implementation log file exporter jira ticket log 1188 #979

pmoogi-redhat · 2021-04-13T05:26:51Z

Description

Summary
Implement an independent process to monitor CRIO container log file rotations that can be used to publish total_bytes_logged metrics in prometheus format. The motivation for moving away from a Ruby implementation is Ruby is not efficient in tracking file rotations. Also collectors can change over time so making metric computation outside of fluentd for logging total_bytes_logged is more preferred.

Exposes metric endpoint that is consumable by Prometheus at port :2112
Exposes metric of total_bytes_logged

/cc @alanconway @jcantrill
/assign @jcantrill

Links

Related PR(s): origin-aggregated-logging:Metric-for-inbound-log-data-loss-at-the-collector-JIRA-ticket-LOG-1032
Got in_tail and prometheus plugin changes which enable total_bytes_collected metric computed and published in prometheus plugin
JIRA: https://issues.redhat.com/browse/LOG-1188

jcantrill

/hold

internal/cmd/go-symlink-logfiles-watcher/symlink-logfiles-watcher.go

internal/go-symlink-logfiles-watcher/Docker/go.mod

internal/go-symlink-logfiles-watcher/Docker/go.sum

internal/go-symlink-logfiles-watcher/Docker/symlink-logfiles-watcher.go

internal/go-symlink-logfiles-watcher/Docker/symnotify.go

internal/go-symlink-logfiles-watcher/hack/build-component-image.sh

internal/cmd/go-symlink-logfiles-watcher/symlink-logfiles-watcher.go

alanconway · 2021-04-14T14:39:28Z

internal/cmd/go-symlink-logfiles-watcher/symlink-logfiles-watcher.go

+	//Get new watcher
+        w := &FileWatcher {
+                metrics: prometheus.NewCounterVec(prometheus.CounterOpts{
+                        Name: "gofilewatcher_input_status_total_bytes_logged",


Another one for @blockloop to weigh in. There are some specific promtail conventions about the order of bytes, total etc. The fluentd metric name also should be consistent wit hthis.

internal/cmd/go-symlink-logfiles-watcher/symlink-logfiles-watcher.go

alanconway · 2021-04-14T14:47:53Z

internal/go-symlink-logfiles-watcher/Docker/Dockerfile

@@ -0,0 +1,17 @@
+FROM openshift/origin-release:golang-1.15 AS build


Do we want a single image with two entrypoints, or two images? I was assuming two images, since we will deploy this as part of the fluentd DaemonSet not with the CLO.

internal/go-symlink-logfiles-watcher/Docker/go.mod

internal/go-symlink-logfiles-watcher/Makefile

alanconway · 2021-04-15T13:36:29Z

Final re-naming! @pmoogi-redhat

Executable: log_exporter

Metrics:

log_logged_bytes_total: total number of bytes written to a single log file path, accounting for rotations.
log_collected_bytes_total: total number of bytes collected from a single log file.
Both metrics have the label "path" which is the absolute path to the log file.

alanconway

Just a few minor renaming diffs now that we've decided on the metric names finally. Otherwise this is good to go.

internal/cmd/logs-files-metric-exporter/symlink-logfiles-watcher.go

jcantrill

please make sure to format before pushing changes make fmt

internal/logs-files-metric-exporter/Docker/Dockerfile

internal/cmd/logs-files-metric-exporter/symlink-logfiles-watcher.go

internal/pkg/symnotify/symnotify.go

jcantrill · 2021-04-15T15:47:41Z

internal/pkg/symnotify/symnotify_test.go

+
+func TestWatchesRealFiles(t *testing.T) {
+	f := NewFixture(t)
+	assert, require := assert.New(t), require.New(t)


Can this be moved into the fixture to reduce code dup

@alanconway seeking your advice here.

internal/pkg/symnotify/symnotify.go

alanconway · 2021-04-16T17:47:36Z

internal/cmd/logs-files-metric-exporter/symlink-logfiles-watcher.go

+                e, err := w.watcher.Event()
+                if err != nil {
+		log.Error(err, "Watcher.Event returning err")
+		os.Exit(1)


I don't know of any recoverable reason for an inotify FD to be closed by the kernel. The inotify object exists indepenently of any files or directories that it is watching.
We could try to re-establish the watch with timed backoff - we don't want to go into a hard CPU spin.

internal/cmd/logs-files-metric-exporter/symlink-logfiles-watcher.go

alanconway · 2021-04-16T18:21:58Z

internal/cmd/logs-files-metric-exporter/symlink-logfiles-watcher.go

+	//Get new watcher
+        w := &FileWatcher {
+                metrics: prometheus.NewCounterVec(prometheus.CounterOpts{
+                        Name: "logs_files_metric_exporter_input_status_bytes_logged_total",


Locked: log_logged_bytes_total

internal/pkg/symnotify/symnotify.go

jcantrill

Please run make fmt before commiting

internal/log-file-metric-exporter/Docker/Dockerfile

internal/cmd/log-file-metric-exporter/main.go

internal/pkg/symnotify/symnotify.go

alanconway · 2021-04-26T17:45:30Z

/approve
I'll let @jcantrill do the LGTM since he's raised a couple more points.

jcantrill

Please run make fmt before sumitting

internal/cmd/log-file-metric-exporter/main.go

internal/pkg/symnotify/symnotify.go

alanconway

Made some suggestions about handling the regexp, otherwise looks good.

internal/cmd/log-file-metric-exporter/main.go

openshift-ci-robot · 2021-04-28T19:04:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pmoogi-redhat
To complete the pull request process, please ask for approval from jcantrill after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Dockerfile

internal/cmd/log-file-metric-exporter/main.go

pkg/k8shandler/fluentd.go

jcantrill · 2021-04-28T19:46:27Z

pkg/k8shandler/fluentd.go

@@ -337,6 +354,26 @@ func (clusterRequest *ClusterLoggingRequest) createOrUpdateFluentdSecret() error
 	return nil
 }

+func newLogMetricExporterContainer() v1.Container {
+	resources := v1.ResourceRequirements{}


@alanconway I left this blank when I provided the PR, but how should we pass in resources here. I'm not fond of opening up the API, but maybe we need to allow a backdoor for setting mem and cpu, maybe via annotations?

We could add a "MetricExporterSpec" to the API, I don't think that would be too bad. The API already has componet-specific requirements (FluentdSpec) so it's not making it any worse.
Alternatively we could split the Fluentd requirements - e.g. give 90% to fluentd and 10% to the exporter. We'd need to do some benchmarks to make sure that's a reasonable split but we should be doing CPU/Mem benehmarks anyway so it's useful work.
The backdoor approach means our collector is using resources that the user doesn't know about, which is not ideal.

I would lean towards just adding to the API. This is something to clean up in the "API v2" effort when we get smarter about scaling and resource use, but it's not worse than whats already there.

@pmoogi-redhat this means we need to add soming like https://github.com/openshift/cluster-logging-operator/blob/master/pkg/apis/logging/v1/clusterlogging_types.go#L35 with the resource requirements. Make sure it additionally includes the comments as these are used for the code generation

@jcantrill @alanconway added changes to support LogFileMetricExportedSpec - please review my changes done in
modified: apis/logging/v1/clusterlogging_types.go
modified: k8shandler/defaults.go
modified: k8shandler/fluentd.go
Hoping above is sufficient for specifying individually resources for LogFileMetricExporter.

@vimalk78 please review as well

pmoogi-redhat

@alanconway incorporated your regex based parsing k8 filename suggestion. please see.

alanconway · 2021-05-03T15:43:46Z

@pmoogi-redhat This all looks good. Add unit tests for FileWatcher, including the file name parsing and I'll LGTM.
Go has very easy-to-use test coverage tools to check your coverage, see https://blog.golang.org/cover

pmoogi-redhat · 2021-07-06T18:11:18Z

/retest

pmoogi-redhat · 2021-07-06T18:12:04Z

/test lint

pmoogi-redhat · 2021-07-07T03:05:27Z

/test lint

pmoogi-redhat · 2021-07-07T03:17:59Z

/test e2e-gcp

pmoogi-redhat · 2021-07-07T08:41:11Z

/test e2e-gcp

pmoogi-redhat · 2021-07-08T08:20:24Z

/test e2e-gcp

pmoogi-redhat · 2021-07-09T04:43:01Z

/test functional

pmoogi-redhat · 2021-07-09T04:43:30Z

/test e2e-gcp

pmoogi-redhat · 2021-07-09T16:49:59Z

/test e2e-gcp

pmoogi-redhat · 2021-07-12T17:47:54Z

/test functional

pmoogi-redhat · 2021-07-13T02:35:39Z

/test e2e-gcp

pmoogi-redhat · 2021-07-13T06:13:01Z

/test e2e-gcp

pmoogi-redhat · 2021-07-13T16:46:21Z

/test e2e-gcp

…pec to expose metric log_logged_bytes_total Delete Makefile.vimal.fix - clean up this Makefile Reverted back to quay.io hosted fluentd image and added comment in fluentd.go Changed back clo pull policy to IfNotPresent merge issue clean up in bundle and manifest csv with https scheme for log-file-metric-exporter + other review comments changes fixed https support issues fixed merge issue in fluentd.go fix to be tested Using fluentdContainer.SecurityContext quay.io hosted ci image for log-file-metric-exporter added in bundle/manifests/cluster-logging.v5.2.0.clusterserviceversion.yaml manifests/5.2/cluster-logging.v5.2.0.clusterserviceversion.yaml taking out /bin/sh -c wrapper from cmd for log-file-metric-exporter taken out dead code on exporterContainer cleanup unnecessary Volume like /usr/local/bin - not required to be mounted varlogcontainers volumemount fix exporterContainer.Command fix - flat string of command line found not working so fixed by providing slice of strings for each argv

pmoogi-redhat · 2021-07-14T10:18:00Z

/test lint

openshift-ci · 2021-07-14T10:23:05Z

@pmoogi-redhat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-operator	8e6327563f4f6a09fb9729c52b6f0cee661ba2ec	link	`/test e2e-operator`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pmoogi-redhat · 2021-07-14T11:01:07Z

/test lint

jcantrill · 2021-07-14T15:45:04Z

/lgtm

openshift-ci · 2021-07-14T15:45:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, jcantrill, pmoogi-redhat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alanconway,jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pmoogi-redhat · 2021-07-15T04:35:40Z

#979 - PR merged on golang based file watcher named as log-file-metric-exporter. It watches log files registered into its watcher object. It publishes log_logged_bytes_total for these log files.

…-log-file-exporter-JIRA-ticket-LOG-1188 Go implementation log file exporter jira ticket log 1188

openshift-ci-robot assigned jcantrill Apr 13, 2021

openshift-ci-robot requested review from alanconway and jcantrill April 13, 2021 05:26

jcantrill reviewed Apr 13, 2021

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2021

alanconway suggested changes Apr 14, 2021

View reviewed changes

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 12c3aae to 4cf01db Compare April 15, 2021 14:05

alanconway suggested changes Apr 15, 2021

View reviewed changes

jcantrill reviewed Apr 15, 2021

View reviewed changes

alanconway mentioned this pull request Apr 16, 2021

Adding logs collected chart LOG-1269 #983

Merged

alanconway suggested changes Apr 16, 2021

View reviewed changes

ajaygupta978 mentioned this pull request Apr 19, 2021

Renaming Metric names and adding namespace label #984

Merged

alanconway reviewed Apr 20, 2021

View reviewed changes

internal/pkg/symnotify/symnotify.go Outdated Show resolved Hide resolved

openshift-ci-robot added the midstream/Dockerfile A Dockerfile.in sync is needed with midstream label Apr 22, 2021

jcantrill requested changes Apr 22, 2021

View reviewed changes

pmoogi-redhat requested a review from alanconway April 23, 2021 10:02

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 55db0b0 to 6908a87 Compare April 26, 2021 08:12

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2021

jcantrill reviewed Apr 26, 2021

View reviewed changes

internal/cmd/log-file-metric-exporter/main.go Outdated Show resolved Hide resolved

internal/pkg/symnotify/symnotify.go Outdated Show resolved Hide resolved

internal/pkg/symnotify/symnotify.go Outdated Show resolved Hide resolved

alanconway suggested changes Apr 28, 2021

View reviewed changes

internal/cmd/log-file-metric-exporter/main.go Outdated Show resolved Hide resolved

internal/cmd/log-file-metric-exporter/main.go Outdated Show resolved Hide resolved

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2021

ajaygupta978 mentioned this pull request Apr 28, 2021

Adding tables for top 10 container producing/collecting logs [Log-1270] [WIP] #994

Closed

jcantrill reviewed Apr 28, 2021

View reviewed changes

pmoogi-redhat commented Apr 30, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2021

pmoogi-redhat mentioned this pull request May 4, 2021

LOG-1213: New plugin metric for inbound log data loss at the collector jira ticket log 1032 openshift/origin-aggregated-logging#2118

Merged

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from e75e742 to ac012ab Compare July 6, 2021 17:38

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from ac012ab to dc12c4f Compare July 7, 2021 16:55

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 9098833 to 92d982c Compare July 8, 2021 14:59

syedriko mentioned this pull request Jul 9, 2021

In the fluentd container security test, account for multiple containers in the fluentd pod #1091

Merged

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 92d982c to 029dc99 Compare July 12, 2021 16:36

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 029dc99 to 5935ef3 Compare July 13, 2021 10:52

pmoogi-redhat force-pushed the Go-implementation-log-file-exporter-JIRA-ticket-LOG-1188 branch from 5935ef3 to e43f772 Compare July 14, 2021 09:13

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021

openshift-merge-robot merged commit 28b7abf into openshift:master Jul 14, 2021

		@@ -0,0 +1,17 @@
		FROM openshift/origin-release:golang-1.15 AS build

Go implementation log file exporter jira ticket log 1188 #979

Go implementation log file exporter jira ticket log 1188 #979

Conversation

pmoogi-redhat commented Apr 13, 2021 • edited by jcantrill

Description

Links

jcantrill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanconway commented Apr 15, 2021 • edited

alanconway left a comment

Choose a reason for hiding this comment

jcantrill left a 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

jcantrill left a comment

Choose a reason for hiding this comment

alanconway commented Apr 26, 2021

jcantrill left a comment

Choose a reason for hiding this comment

alanconway left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 28, 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

pmoogi-redhat left a comment

Choose a reason for hiding this comment

alanconway commented May 3, 2021

pmoogi-redhat commented Jul 6, 2021

pmoogi-redhat commented Jul 6, 2021

pmoogi-redhat commented Jul 7, 2021

pmoogi-redhat commented Jul 7, 2021

pmoogi-redhat commented Jul 7, 2021

pmoogi-redhat commented Jul 8, 2021

pmoogi-redhat commented Jul 9, 2021

pmoogi-redhat commented Jul 9, 2021

pmoogi-redhat commented Jul 9, 2021

pmoogi-redhat commented Jul 12, 2021

pmoogi-redhat commented Jul 13, 2021

pmoogi-redhat commented Jul 13, 2021

pmoogi-redhat commented Jul 13, 2021

pmoogi-redhat commented Jul 14, 2021

openshift-ci bot commented Jul 14, 2021 • edited

pmoogi-redhat commented Jul 14, 2021

jcantrill commented Jul 14, 2021

openshift-ci bot commented Jul 14, 2021

pmoogi-redhat commented Jul 15, 2021

pmoogi-redhat commented Apr 13, 2021 •

edited by jcantrill

alanconway commented Apr 15, 2021 •

edited

openshift-ci bot commented Jul 14, 2021 •

edited