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

Implement logging telemetry log 1926 #1264

Conversation

pmoogi-redhat
Copy link
Contributor

Description

Story
As a member of the OpenShift Logging Team, I want telemetry metrics of the CLO
so that we have a better understanding of how CLF is being utilized by customers (e.g. which outputs)

Acceptance Criteria
Verifying telemetry metrics are available from the CLO metrics endpoint in prometheus

/cc @jcantrill @alanconway
/assign

Links

@pmoogi-redhat
Copy link
Contributor Author

/test functional

internal/k8shandler/reconciler.go Outdated Show resolved Hide resolved
internal/k8shandler/reconciler.go Outdated Show resolved Hide resolved
internal/telemetry/clotelemetrymetric.go Outdated Show resolved Hide resolved
internal/telemetry/clotelemetrymetric.go Outdated Show resolved Hide resolved
}

var (
ClTelData = New()
Copy link
Contributor

Choose a reason for hiding this comment

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

Global variable is not good practice, make this a member of the reconciler or some other suitable home.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanconway i see that EO also implemented by declaring global variables part of their metrics package. i wanted to do minimal changes to existing CLO stack and keep all new changes / implementation confining to this telemetry package. do you still think we should make this data a member of reconciler ? let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is a concurrent-unsafe pattern. Even if it is implemented safely today it is much too easy for someone who doesn't understand the concurrency rules to make an unsafe reference in new code.

Lets leave it global but use a concurrent-safe map for now. Patch below for a concurrent-safe string map.
I'd like to review the pattern in future, but for now that should do the trick.

diff --git a/internal/utils/stringmap.go b/internal/utils/stringmap.go
new file mode 100644
index 000000000..de1e10bc9
--- /dev/null
+++ b/internal/utils/stringmap.go
@@ -0,0 +1,28 @@
+package utils
+
+import "sync"
+
+// StringMap is a concurrent-safe map[string]string based on sync.Map
+type StringMap struct{ m sync.Map }
+
+// GetOK returns (value, true) if key is present and ("", false) if absent.
+func (m *StringMap) GetOK(key string) (s string, ok bool) {
+	x, ok := m.m.Load(key)
+	if ok {
+		s = x.(string)
+	}
+	return s, ok
+}
+
+// Get returns the value, or "" if the key is absent.
+func (m *StringMap) Get(key string) string {
+	if s, ok := m.GetOK(key); ok == true {
+		return s
+	}
+	return ""
+}
+
+// Set sets key to value.
+func (m *StringMap) Set(key, value string) {
+	m.m.Store(key, value)
+}
diff --git a/internal/utils/stringmap_test.go b/internal/utils/stringmap_test.go
new file mode 100644
index 000000000..ed008f6c7
--- /dev/null
+++ b/internal/utils/stringmap_test.go
@@ -0,0 +1,20 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestStringMap(t *testing.T) {
+	m := StringMap{}
+	assert.Equal(t, "", m.Get("x"))
+	s, ok := m.GetOK("x")
+	assert.Equal(t, "", s)
+	assert.False(t, ok)
+	m.Set("x", "y")
+	assert.Equal(t, "y", m.Get("x"))
+	s, ok = m.GetOK("x")
+	assert.Equal(t, "y", s)
+	assert.True(t, ok)
+}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanconway resolved

internal/telemetry/clotelemetrymetric.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
internal/telemetry/clotelemetrymetric.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2021
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch from 4718839 to 729a1c7 Compare December 6, 2021 08:21
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2021
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch from 729a1c7 to d9211c6 Compare December 6, 2021 08:38
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 2 times, most recently from 5bd9862 to d77f402 Compare December 14, 2021 12:32
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2021
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch from 3349b37 to d5704a4 Compare December 20, 2021 16:06
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2021
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 3 times, most recently from 58412aa to 79f91d6 Compare January 5, 2022 06:45
@pmoogi-redhat
Copy link
Contributor Author

/test e2e

@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 3 times, most recently from 2a2486c to 33cc2fa Compare January 8, 2022 11:09
@pmoogi-redhat
Copy link
Contributor Author

/test e2e

1 similar comment
@pmoogi-redhat
Copy link
Contributor Author

/test e2e

@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 3 times, most recently from bc4fdfe to 52402af Compare January 11, 2022 13:48
@pmoogi-redhat
Copy link
Contributor Author

/test e2e

@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 2 times, most recently from 975e42c to 4d97ad8 Compare January 14, 2022 11:45
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch 2 times, most recently from 54c85b9 to 0de832b Compare January 19, 2022 04:31
@pmoogi-redhat pmoogi-redhat force-pushed the Implement-logging-telemetry-LOG-1926 branch from 0de832b to 07c9767 Compare January 19, 2022 13:52
@pmoogi-redhat
Copy link
Contributor Author

/test e2e

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 19, 2022

@pmoogi-redhat: all tests passed!

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.

Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, 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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2022
@openshift-merge-robot openshift-merge-robot merged commit 36a1f9b into openshift:master Jan 20, 2022
pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022
…g-telemetry-LOG-1926

Implement logging telemetry log 1926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants