Redis receiver #138

pmcollins · 2020-03-13T17:20:30Z

Description: Add Redis receiver.

Todo: 1) Error handling: currently bailing out on any error. Need to add resiliency. 2) Improved organization: everything is in one sprawling package. 3) Verification of Redis metric types. 4) Licensing headers. 5) Comments. 6) Logging.

pmcollins · 2020-03-13T17:26:17Z

receiver/redisreceiver/metric_functions.go

@@ -0,0 +1,231 @@
+package redisreceiver
+
+func uptimeInSeconds() *redisMetric {


It has been suggested that I turn this data into a map. Looking into it.

pmcollins · 2020-03-13T17:28:02Z

receiver/redisreceiver/metrics_service_test.go

+	require.Nil(t, err)
+	require.Equal(t, 29, len(md.Metrics))
+}
+


Need to test error/failure handling as well.

pmcollins · 2020-03-13T17:28:56Z

receiver/redisreceiver/receiver.go

+}
+
+func (r redisReceiver) Start(host component.Host) error {
+	r.Lock()


I modeled this on the other receivers but would like to learn more about why the mutex and startOnce are needed.

It should not be needed. Start is called once only.

@pjanotti any thoughts on this? I saw the same pattern in signalfxreceiver, so perhaps there is something I am missing.

(We do have "once" protection in opencensusreceiver, but there it is needed because it implements trace and metrics receiver via single instance and internal start function may be called twice. Everywhere else I do not think we need such protection).

Ok, will remove for now. Thanks.

flands · 2020-03-13T17:35:16Z

receiver/redisreceiver/config.go

+import "github.com/open-telemetry/opentelemetry-collector/config/configmodels"
+
+type Config struct {
+	configmodels.ReceiverSettings `mapstructure:",squash"`


Please add a README with configuration information -- see other receivers for an example

Sounds good. Will do.

tigrannajaryan

@pmcollins Nice work. I left a few comments.

A general concern about the code is lack of explanatory comments. Would be great to add some.

tigrannajaryan · 2020-03-13T19:22:33Z

receiver/redisreceiver/converters.go

+import (
+	"strconv"
+
+	metricsProto "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"


We usually alias this import to metricspb elsewhere in the codebase.

Suggested change

metricsProto "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"

metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"

tigrannajaryan · 2020-03-13T19:23:14Z

receiver/redisreceiver/api_parser.go

+
+import "strings"
+
+type apiParser struct {


Please add comments to explain the purpose of non-trivial structs and functions.

tigrannajaryan · 2020-03-13T19:26:03Z

receiver/redisreceiver/metrics_service.go

+package redisreceiver
+
+import (
+	resourceProto "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"


For uniformity with the rest of the codebase:

Suggested change

resourceProto "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"

resourcepb "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"

tigrannajaryan · 2020-03-13T19:30:35Z

receiver/redisreceiver/receiver.go

+}
+
+func (r redisReceiver) Start(host component.Host) error {
+	r.Lock()


It should not be needed. Start is called once only.

@pjanotti any thoughts on this? I saw the same pattern in signalfxreceiver, so perhaps there is something I am missing.

(We do have "once" protection in opencensusreceiver, but there it is needed because it implements trace and metrics receiver via single instance and internal start function may be called twice. Everywhere else I do not think we need such protection).

tigrannajaryan · 2020-03-13T19:42:01Z

receiver/redisreceiver/metric_functions.go

+func uptimeInSeconds() *redisMetric {
+	return &redisMetric{
+		key:        "uptime_in_seconds",
+		units:      "seconds",


Since this eventually gets assigned to MetricDescriptor.Unit it needs to follow conventions defined in http://unitsofmeasure.org/ucum.html

@bogdandrutu do I understand it correct that we should use "c/s" form for unit, i.e. for seconds we should use "s". Is that right?

I kind of punted on the units for this draft, but will make sure they are consistent with the spec.

tigrannajaryan · 2020-03-13T19:44:41Z

receiver/redisreceiver/redis_metrics.go

+type metricType int
+
+const (
+	unspecified = iota
+	gaugeInt
+	gaugeDouble
+	gaugeDistribution
+	cumulativeInt
+	cumulativeDouble
+)


Is there any reason that we redeclare this instead of using MetricDescriptor_Type type? The code elsewhere assumes they must be equivalent and uses just type-cast for converting between metricType and MetricDescriptor_Type. In that case what do we gain by having this separate declaration?

I wanted to keep this independent of protobuf because it was my understanding that there would be a new data model coming soon that was encoding agnostic, so I didn't want to potentially have to change these values in a bunch of places. Happy to use MetricDescriptor_Type directly though if you think that would be better at this time.

Thanks, I understand now the intent. However, because we simply use type conversion between metricType and MetricDescriptor_Type they are inherently tightly coupled although it may not appear so when glancing at the code. If the intent is to make redis metric declarations encoding agnostic then it would be better to have conversion function from metricType to MetricDescriptor_Type and avoid using type conversion.
That is likely an overkill though. We are likely fine with relying on internal data types (and we can assume MetricDescriptor_Type is an internal data type - although the assumption is temporary) and just use MetricDescriptor_Type directly. Once we have the new internal data types we can just update the declaration here. When that happens the compiler will tell us if there are any type mismatches elsewhere, but only if we avoid using type conversions.

Anyway, this is a minor nit for now. Eventually I believe we should feel free to use internal data types within our codebase.

Sounds good. Replaced with MetricDescriptor_Type.

tigrannajaryan · 2020-03-13T19:48:40Z

receiver/redisreceiver/redis_metrics.go

+func buildSingleProtoMetric(strVal string, redisMetric *redisMetric) (*metricsProto.Metric, error) {
+	f := getPointConverter(redisMetric)
+	if f == nil {
+		return nil, errors.New("oops") // todo constant?


Presumably this can never happen because we don't have redisMetrics of any type that is not an int or double. Can we somehow ensure that if we do we can a compile error or we have a test that ensures this is not possible? This would prevent future problems if someone adds a redisMetric with a different type to getDefaultRedisMetrics.

pmcollins · 2020-03-16T14:00:42Z

@pmcollins Nice work. I left a few comments.

A general concern about the code is lack of explanatory comments. Would be great to add some.

Thanks for the review. Will add comments.

Todo: 1) Error handling: currently bailing out on any error. Need to add resiliency. 2) Improved organization: everything is in one sprawling package. 3) Verification of Redis metric types. 4) Licensing headers. 5) Comments. 6) Logging.

…lector-contrib into redis

receiver/redisreceiver/README.md

pmcollins · 2020-03-23T15:26:31Z

I signed it

receiver/redisreceiver/README.md

receiver/redisreceiver/client.go

receiver/redisreceiver/keyspace.go

receiver/redisreceiver/proto.go

receiver/redisreceiver/redis_runnable.go

receiver/redisreceiver/redis_metric.go

receiver/redisreceiver/info.go

Co-Authored-By: Paulo Janotti <pjanotti@splunk.com>

bogdandrutu · 2020-04-02T20:26:57Z

receiver/redisreceiver/proto.go

+}
+
+// TODO: Maybe this should be moved to a general purpose utility if we don't have one already
+func timeToTimestamp(t *time.Time) *timestamp.Timestamp {


You can use this: https://godoc.org/github.com/golang/protobuf/ptypes#TimestampProto

Nice, fixed.

bogdandrutu · 2020-04-02T20:37:22Z

receiver/redisreceiver/proto.go

+
+	if redisMetric.mdType == metricspb.MetricDescriptor_CUMULATIVE_INT64 ||
+		redisMetric.mdType == metricspb.MetricDescriptor_CUMULATIVE_DOUBLE {
+		startTime = &timestamp.Timestamp{} // todo: not sure about this


I think we should approximate the startTime as:
Now() - uptime_in_seconds

We should do this only the first time we query the metrics because of the network delay/clock skew we may not get the same number all the time, but the approximation I proposed is good enough.

And as a bonus we can calculate the current time not as the Time.Now() but as startTime + uptime_in_seconds - first_scrape_uptime_in_seconds (got at first scrape). This way we avoid network delays problems.

If we do this trick with start_time we should track when uptime_in_seconds < last_scrape_uptime_in_seconds and recalculate the startime and update the first_scrape_uptime_in_seconds (if you do the bonus part).

Nice, thanks. I have made a change to track the server start time as you suggest.

tigrannajaryan · 2020-04-04T00:34:37Z

Please use imperative mood in the subject line of commit message first line and add more details to both commit message and PR description to explain what the change does.

If you didn't yet have a chance please read:

The general contributing guidelines at https://github.com/open-telemetry/community/blob/master/CONTRIBUTING.md
Collector contributing guidelines at https://github.com/open-telemetry/opentelemetry-collector/blob/master/CONTRIBUTING.md
How to write good commit messages at https://chris.beams.io/posts/git-commit/

tigrannajaryan · 2020-04-04T00:35:14Z

receiver/redisreceiver/README.md

+instance, build metrics from that data, and send them to the next consumer at a
+configurable interval.
+
+Status: beta


tigrannajaryan · 2020-04-04T00:39:35Z

receiver/redisreceiver/redis_runnable.go

+	return nil
+}
+
+// Runs intermittently, querying Redis and building Metrics to send to the


Runs intermittently

This is a bit confusing. This function performs a single run, single cycle of collection from Redis, right? When I read the comment I get the impression that Run() function internally performs multiple run cycles.

Perhaps reword a bit, like this:

// Run is called periodically, ...

Better, fixed.

tigrannajaryan · 2020-04-04T00:40:46Z

receiver/redisreceiver/redis_svc.go

+			continue
+		}
+		pair := strings.Split(line, ":")
+		if len(pair) == 2 { // defensive, should always == 2


tigrannajaryan · 2020-04-04T00:41:12Z

receiver/redisreceiver/README.md

+The Redis receiver is designed to retrieve Redis INFO data from a single Redis
+instance, build metrics from that data, and send them to the next consumer at a
+configurable interval.


Nice overview.

tigrannajaryan · 2020-04-04T00:42:05Z

receiver/redisreceiver/README.md

+The Redis INFO command returns information and statistics about a Redis
+server (see [https://redis.io/commands/info](https://redis.io/commands/info) for
+details).
+The Redis receiver extracts values from the result and converts them to open
+telemetry metrics. Details about the metrics produced by the Redis receiver
+can be found by browsing [metric_functions.go](metric_functions.go).
+
+For example, one of the fields returned by the Redis INFO command is
+`used_cpu_sys` which indicates the system CPU consumed by the Redis server,
+expressed in seconds, since the start of the Redis instance.


Nit: please use consistent line spacing for paragraphs (the first 2 paragraphs do not have an empty line between).

Done (it's just one paragraph).

tigrannajaryan · 2020-04-04T00:43:40Z

receiver/redisreceiver/README.md

+
+_Required._
+
+### refresh_interval


Is it collection_interval or refresh_interval?

Should be collection interval -- I renamed the field and missed this file. Fixed.

Cumulative metrics have an implied start time. This change keeps track of the server start time so it can be applied to cumulative metrics. Also fixes issues raised in PR feedback.

* Use named return values for errors that should be treated as warnings * Handle malformed keyspace key value pairs * Update README to use env var in example config * Remove unnecessary EndMetricsReceiveOp in redis runnable

pjanotti

LGTM. Thanks @pmcollins!

* Run golangci-lint in all directories with go.mod * Imports fix by golangci-lint * Propagate failures from commands running in loops

…ce not set open-telemetry#95" (open-telemetry#138)

Pablo Collins added 4 commits March 12, 2020 12:52

Work in Progress: Redis receiver

8e1691a

Todo: 1) Error handling: currently bailing out on any error. Need to add resiliency. 2) Improved organization: everything is in one sprawling package. 3) Verification of Redis metric types. 4) Licensing headers. 5) Comments. 6) Logging.

wip: redis receiver: fix typo, use slice literal

aeabc15

redis receiver: add logging, error handling, thread safety

01ae6af

redis receiver: rename stragging 'tickable' variables to runnable

cb0759a

pmcollins commented Mar 13, 2020

View reviewed changes

flands reviewed Mar 13, 2020

View reviewed changes

tigrannajaryan reviewed Mar 13, 2020

View reviewed changes

Pablo Collins added 8 commits March 19, 2020 11:38

Update Redis receiver based on prelim feedback

43f0443

Work in Progress: Redis receiver

5928567

Todo: 1) Error handling: currently bailing out on any error. Need to add resiliency. 2) Improved organization: everything is in one sprawling package. 3) Verification of Redis metric types. 4) Licensing headers. 5) Comments. 6) Logging.

wip: redis receiver: fix typo, use slice literal

56b2cfa

redis receiver: add logging, error handling, thread safety

92dda68

redis receiver: rename stragging 'tickable' variables to runnable

8b507e6

Update Redis receiver based on prelim feedback

e01e544

Add comments to Redis receiver

3e578f7

Merge branch 'redis' of https://github.com/signalfx/opentelemetry-col…

671fd1f

…lector-contrib into redis

jrcamp reviewed Mar 19, 2020

View reviewed changes

receiver/redisreceiver/README.md Show resolved Hide resolved

Pablo Collins added 4 commits March 20, 2020 10:39

Run go fmt on redis receiver

0b6915d

Add copyright to redisreceiver files

b5ce286

Go fmt redisreceiver

c659132

Fix import groupings for make ci

ec28707

Refactor and cleanup lint issues

f4a776b

pmcollins marked this pull request as ready for review March 23, 2020 18:32

pmcollins requested review from bogdandrutu, owais, pjanotti and rghetia as code owners March 23, 2020 18:32

pmcollins requested a review from songy23 as a code owner March 23, 2020 18:32

pmcollins and others added 3 commits March 25, 2020 09:34

Merge branch 'master' into redis

45d0904

Redis receiver otelcol version bump

4c5bd44

Redis receiver tweaks

d677bfe

pjanotti reviewed Mar 25, 2020

View reviewed changes

Pablo Collins and others added 7 commits March 26, 2020 10:53

Update receiver/redisreceiver/README.md

26595a1

Co-Authored-By: Paulo Janotti <pjanotti@splunk.com>

Redis receiver: Expand README, add obsreport metrics.

6093d2f

Tweak Redis runnable test formatting

00929ce

Merge branch 'master' into redis

372b5f7

Update redis collector to use latest otelcol api

b2e36ee

Merge branch 'master' into redis

83bc818

Change redis refresh interval to collection interval

8bdfacd

bogdandrutu reviewed Apr 2, 2020

View reviewed changes

tigrannajaryan reviewed Apr 4, 2020

View reviewed changes

Pablo Collins and others added 6 commits April 6, 2020 12:02

Add server start time to cumulative metrics

465ad18

Cumulative metrics have an implied start time. This change keeps track of the server start time so it can be applied to cumulative metrics. Also fixes issues raised in PR feedback.

Merge branch 'master' into redis

5259686

Fix redis receiver issues from code review

7d97aba

* Use named return values for errors that should be treated as warnings * Handle malformed keyspace key value pairs * Update README to use env var in example config * Remove unnecessary EndMetricsReceiveOp in redis runnable

Merge branch 'master' into redis

3dfe553

Update go sum for redis

09e8a0a

Update otelcol version for Redis receiver

e274a9a

pjanotti approved these changes Apr 8, 2020

View reviewed changes

pjanotti merged commit 7fd9a76 into open-telemetry:master Apr 8, 2020

pmcollins mentioned this pull request May 8, 2020

REQUEST: New membership for pmcollins open-telemetry/community#345

Closed

6 tasks

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

run golangci-lint in all directories with go.mod (#138)

908b73d

* Run golangci-lint in all directories with go.mod * Imports fix by golangci-lint * Propagate failures from commands running in loops

XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Feb 9, 2024

Reverting "Publishing 0 for cpu/memory over metrics when limit/resour…

505e232

…ce not set open-telemetry#95" (open-telemetry#138)

		@@ -0,0 +1,231 @@
		package redisreceiver

		func uptimeInSeconds() *redisMetric {

	metricsProto "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"

	resourceProto "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"
	resourcepb "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"


		_Required._

		### refresh_interval

Redis receiver #138

Redis receiver #138

Conversation

pmcollins commented Mar 13, 2020

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

tigrannajaryan 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

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

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

pmcollins commented Mar 16, 2020

pmcollins commented Mar 23, 2020

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

tigrannajaryan commented Apr 4, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment