Add semantic conventions for redis receiver #2145

manang-splunk · 2021-11-22T05:51:19Z

Changes

Added new resource attribute redis.instance for redis receiver.

arminru · 2021-11-22T09:49:42Z

specification/resource/semantic_conventions/redis.md

+<!-- semconv redis -->
+| Attribute  | Type | Description  | Examples  | Required |
+|---|---|---|---|---|
+| `redis.instance` | string | Reported name of the redis instance | `localhost:6379` | No |


Hi @manang-splunk! Could you please elaborate on how this attribute would be used?
As this is a resource attribute, it would have to be populated by the Redis instance itself, i.e., an OpenTelemetry instrumentation built into Redis server directly. If this is your intention, please clarify it accordingly.

redis.instance attribute would be used as a resource attribute whose value is configurable by user. This attribute was missing from redis metrics so we added it as resource attribute.

@arminru Please share your thoughts on this.

@manang-splunk With Redis metrics you are referring to @codeboten's open PR #2070 and this collector plugin, right?
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/redisreceiver

@arminru I am referring to PR #5922 where we have added a resource attribute redis.instance in code and we are simply adding its semantic convention for the same in this PR. Please go through PR 5922 to understand the usage of redis.instance attribute.

Thanks for the reference @manang-splunk, now it makes more sense to me!
I wonder if the service.name and service.instance.id resource attributes defined here and suggested by @bogdandrutu on your PR would be sufficient or not, have you looked into these? They would be used to describe a "logical" name for the service and are expected to be provided by users.
If the value is supposed to describe the host on which the Redis instance is running like you indicate in your example, the host.name attribute defined here could be suitable but having the port in there might be unexpected, even if the definition says "or another name specified by the user".
@bogdandrutu WDYT?

Hello @arminru ,
CC: @bogdandrutu

The current task is to add a resource attribute in the Redis Receiver similar to what is existing in the SignalFx agent Collectd/Redis monitor.

The SignalFx agent Collectd/Redis monitor has a plugin_instance attribute which expects a value in the format of {host}:{port} or a normal string. This helps to label the metrics with user-configured values. Similar functionality does not exist today in the Redis receiver and needs to be implemented as a part of the current task.

To implement this functionality, we have the below two possibilities :

Option 1 - We can leverage existing resource attributes in the Redis receiver such that it can accommodate the value of both the formats {host}:{port} or a normal string based on the above requirements. We have three attributes, service.name, service.instance.id and host.name but if {host}:{port} format of value gets populated within these existing attributes, then it would seem unexpected as the data format {host}:{port} is more like an endpoint type of data rather than service-name-related, service-instance-id-related or only host-name-related data. Hence, this option is not suggested.

Option 2 - We can introduce a new resource attribute called redis.instance which can accommodate both these formats {host}:{port} or a normal string. Hence, this option is suggested and this logical workflow is implemented as a part of this task.

Please let us know your inputs or queries.

Thanks for the explanation.
I looked at service.instance.id again and agree that it is not really suitable as it is intended to distinguish multiple instances of the same service, so this would only be applicable for Redis cluster nodes.
Having just the form host:port for service.name is not ideal indeed as this does not correspond well with the definition of a logical service name. If we'd add redis.instance, which service.name would the Redis receiver set?

I'm open for adding redis.instance if we don't see a good fit with the existing attributes but would like to get more opinions from the other @open-telemetry/specs-approvers. This would be the first technology/framework-specific resource attribute to be added and I wonder if we were already able to cover similar cases with the existing, generic attributes.

The usage of service.name attribute is user specific. Based on the observed data of the Redis server, there is no service-name-related data and as per our usecase host:port doesn't fit with service.name. Hence, we are not setting the service.name attribute. I have attached the sample data we observed from the Redis server (data.txt).

arminru · 2021-12-02T16:38:33Z

specification/resource/semantic_conventions/redis.md

+<!-- semconv redis -->
+| Attribute  | Type | Description  | Examples  | Required |
+|---|---|---|---|---|
+| `redis.instance` | string | Reported name of the redis instance | `localhost:6379` | No |


Thanks for the explanation.
I looked at service.instance.id again and agree that it is not really suitable as it is intended to distinguish multiple instances of the same service, so this would only be applicable for Redis cluster nodes.
Having just the form host:port for service.name is not ideal indeed as this does not correspond well with the definition of a logical service name. If we'd add redis.instance, which service.name would the Redis receiver set?

specification/resource/semantic_conventions/redis.md

semantic_conventions/resource/redis.yaml

arminru · 2021-12-02T16:46:39Z

semantic_conventions/resource/redis.yaml

+      - id: instance
+        type: string
+        brief: >
+          Reported name of the redis instance


Suggested change

Reported name of the redis instance

Reported name of the Redis instance. This can be in the form `{host}:{port}` or any other name provided manually when setting up the instrumentation.

Not yet happy with my suggestion but this lacks clarification. I'm open for better suggestions :)

Done, added an appropriate description.

tigrannajaryan · 2021-12-02T17:29:09Z

We have conventions for databases: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md which uses prefix db.redis.* for Redis attributes. Should this attribute be under db.redis.*?

Side note: I am not sure it is a good idea to have db.<dbname>.* namespace when we also have db.* namespace. This has a lot of potential for name clashes. These conventions may need to be revamped.

hprajapati-splunk · 2021-12-14T17:12:21Z

We have conventions for databases: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md which uses prefix db.redis.* for Redis attributes. Should this attribute be under db.redis.*?

Side note: I am not sure it is a good idea to have db.<dbname>.* namespace when we also have db.* namespace. This has a lot of potential for name clashes. These conventions may need to be revamped.

Hello @tigrannajaryan,
We have added redis.instance as a resource attribute. The database semantics you referred are for trace attributes, which are used for database client calls. If we add db.redis.* in the resource attribute then it will have conflicts with db.* namespace as you mentioned earlier, so we have added redis.instance (it can be an endpoint or any string value provided by the user). You can refer this conversation for more details about the field we have added(redis.instance).

Please let us know your thoughts.

tigrannajaryan · 2021-12-15T14:19:19Z

The database semantics you referred are for trace attributes, which are used for database client calls. If we add db.redis.* in the resource attribute then it will have conflicts with db.* namespace as you mentioned earlier

I do not see any reason to limit db.* attributes to traces only. They can be also valid for metrics describing databases and I would expect for example db.system or db.name to be recorded in the Resource for metrics. I think we can have db.* namespace in the Resource, which can also contain db.redis.instance attribute.

This raises a good question though: how do we define semantic conventions for attributes that may be recorded both at the Resource level and at the Span or Metric level? I don't think Otel spec has a good answer for this and we have an open issue about that #1367

To make progress on this I suggest the following. Change this PR to use db.redis.instance. I do not think top-level redis.* is acceptable. We can accept this as experimental convention. Be prepared that the convention may be changed when #1367 is resolved.

manang-splunk · 2021-12-16T13:28:31Z

The database semantics you referred are for trace attributes, which are used for database client calls. If we add db.redis.* in the resource attribute then it will have conflicts with db.* namespace as you mentioned earlier

I do not see any reason to limit db.* attributes to traces only. They can be also valid for metrics describing databases and I would expect for example db.system or db.name to be recorded in the Resource for metrics. I think we can have db.* namespace in the Resource, which can also contain db.redis.instance attribute.

This raises a good question though: how do we define semantic conventions for attributes that may be recorded both at the Resource level and at the Span or Metric level? I don't think Otel spec has a good answer for this and we have an open issue about that #1367

To make progress on this I suggest the following. Change this PR to use db.redis.instance. I do not think top-level redis.* is acceptable. We can accept this as experimental convention. Be prepared that the convention may be changed when #1367 is resolved.

We tried to add db.redis.instance with namespace as db.* as well as with db.redis.* at the resource level, we got the following error while running make table-generation command as the namespace already exists at the trace level:

Error parsing /source/trace/database.yaml

Semantic convention 'db' is already defined.

Error parsing /source/trace/database.yaml

Semantic convention 'db.redis' is already defined.

Is there something that we are missing from our end?

tigrannajaryan · 2021-12-20T16:07:25Z

@manang-splunk I am not sure what the problem is, perhaps the semantic convention generator has limitations that prevent having the same namespace for resources and traces. This may require fixing the generator first. @open-telemetry/build-tools-approvers any thoughts?

github-actions · 2021-12-28T03:17:13Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-01-11T03:17:42Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan · 2022-01-12T20:43:35Z

@manang-splunk can you create an issue describing the limitation of the semantic convention generator tool, along with a proposal of how to fix it (if you have one)?

manang-splunk · 2022-01-13T14:04:17Z

@manang-splunk can you create an issue describing the limitation of the semantic convention generator tool, along with a proposal of how to fix it (if you have one)?

@tigrannajaryan As we were setting group id as “db.redis” and it was already used at trace level we were getting error that “db.redis” is already there. So we used different group id as “redis” for this. Now we are able to add the “db.redis.instance” as a resource semantic convention. We have pushed the changes accordingly can you please review it?

As it is a type of database, we have created a folder “db” (semantic_conventions/resource/db) and kept conventions of Redis under that. Any other database conventions can be added under that in future.

github-actions · 2022-01-21T03:16:59Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan

The namespace and the name of the new attribute look good to me.
I do not know much of Redis to tell if the attribute value is what we want to record. I would like others to chime in too.

tigrannajaryan · 2022-01-21T14:46:06Z

@open-telemetry/specs-approvers @open-telemetry/instr-wg please review.

joaopgrassi

I'm not familiar with the Redis receiver but I went through the PRs and I think this makes sense. Just some editorial suggestions and a question about the structure of the convention.

joaopgrassi · 2022-01-22T11:41:46Z

semantic_conventions/resource/db/redis.yaml

@@ -0,0 +1,14 @@
+groups:
+  - id: redis
+    prefix: db.redis


I quickly searched and I didn't find another convention where the id is at a "different level" than the prefix. I'm not sure if this is a problem though, just something that stand up for me.

I guess you will also get a conflict if you have a structure like this: id: db > id: db.redis > attributes > id: instance? Similar to this

opentelemetry-specification/semantic_conventions/trace/database.yaml

Line 237 in d7a6b9f

- id: db.mssql

?

Yes, I was getting conflicts while using id as db and db.redis.
As we already have db and db.redis as id value in database attributes, we cannot use the same here. Therefore we have kept id as redis.

joaopgrassi · 2022-01-22T11:44:41Z

semantic_conventions/resource/db/redis.yaml

+      - id: instance
+        type: string
+        brief: >
+          Reported name of the Redis instance. This can be in the form


Suggested change

Reported name of the Redis instance. This can be in the form

The reported name of the Redis instance. This can be in the form of

Pushed the changes accordingly.

joaopgrassi · 2022-01-22T11:47:18Z

semantic_conventions/resource/db/redis.yaml

+        brief: >
+          Reported name of the Redis instance. This can be in the form
+          `{host}:{port}` or any other name provided manually while configuring
+          the instrumentation and defaults to the `endpoint` value provided in the


I think the sentence is a bit too long and hard to follow. A suggestion would be maybe ending the sentence in the instrumentation. and starting a new one with. The default value is the endpoint... or When not provided, the default value is.... What do you think?

Pushed the changes accordingly.

joaopgrassi · 2022-01-22T12:49:14Z

specification/resource/semantic_conventions/db/redis.md

@@ -0,0 +1,13 @@
+# Redis
+
+**Status**: [Experimental](../../document-status.md)


The link is missing one more level I think (that's why the docfx check is failing)

Pushed the changes accordingly.

joaopgrassi · 2022-01-24T16:04:17Z

Sorry for the back and forth, but I'm not sure the current solution (due to the conflict in the namespace) is the best. As @tigrannajaryan mentioned, we might need to change the build-tool or how we invoke it.

I tried something quickly and I managed to call the table-generation without the conflict, but it's also a bit "hackish". If you change your redis.yaml to have

id: db.redis
    prefix: db.redis
    ...

Then running the make table-generation will fail with the conflict. As far as I could see, that's because the command invokes the generation tool passing the whole semconv folder which contains traces and resources. If you change it, passing the resource folder alone it works. Like this:

# Generate markdown tables from YAML definitions
.PHONY: table-generation
table-generation:
   docker run --rm -v $(PWD)/semantic_conventions/resource:/source -v $(PWD)/specification/resource:/spec \
   	otel/semconvgen:$(SEMCONVGEN_VERSION) -f /source markdown -md /spec

This will generate the table on db/redis.md correctly.

I'm not sure if this is a feasible solution though. Maybe @Oberon00 can chime in and share what he thinks about it. Maybe we will really need to change the generator.

github-actions · 2022-02-08T03:17:08Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-02-15T03:17:11Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

manang-splunk requested review from a team November 22, 2021 05:51

github-actions bot assigned arminru Nov 22, 2021

arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Nov 22, 2021

arminru reviewed Nov 22, 2021

View reviewed changes

arminru requested a review from a team November 22, 2021 09:51

arminru reviewed Dec 2, 2021

View reviewed changes

manang-splunk force-pushed the main branch from 411ec24 to e44d562 Compare December 7, 2021 04:50

manang-splunk requested a review from arminru December 9, 2021 12:55

manang-splunk force-pushed the main branch from e44d562 to 27319f8 Compare December 13, 2021 10:31

arminru requested review from bogdandrutu and a team December 13, 2021 16:35

tigrannajaryan mentioned this pull request Dec 15, 2021

Move semantic conventions that are applicable to the Resource from "trace" spec to a common area #1367

Open

github-actions bot added the Stale label Dec 28, 2021

bogdandrutu removed the Stale label Jan 4, 2022

github-actions bot added the Stale label Jan 11, 2022

tigrannajaryan mentioned this pull request Jan 12, 2022

Add semantic conventions for k8s #2258

Closed

github-actions bot removed the Stale label Jan 13, 2022

manang-splunk force-pushed the main branch 2 times, most recently from 7fd7a95 to 0ad251c Compare January 13, 2022 13:22

github-actions bot added the Stale label Jan 21, 2022

tigrannajaryan approved these changes Jan 21, 2022

View reviewed changes

tigrannajaryan removed the Stale label Jan 21, 2022

joaopgrassi reviewed Jan 22, 2022

View reviewed changes

manang-splunk force-pushed the main branch from 0ad251c to 9faf40f Compare January 24, 2022 11:42

manang-splunk requested a review from joaopgrassi January 24, 2022 12:03

manang-splunk added 4 commits January 31, 2022 18:08

Add semantic conventions for redis receiver

6ce30e6

update CHANGELOG.md and README.md for redis resource semantic convention

db31915

added db.redis.instance semantic convention

f920bb8

added db.redis.instance attribute

e5f6243

manang-splunk force-pushed the main branch from 9faf40f to e5f6243 Compare January 31, 2022 12:40

github-actions bot added the Stale label Feb 8, 2022

github-actions bot closed this Feb 15, 2022

This was referenced Sep 26, 2022

Add redis resource #2836

Closed

Add a redis resource #2840

Closed

	Reported name of the redis instance
	Reported name of the Redis instance. This can be in the form `{host}:{port}` or any other name provided manually when setting up the instrumentation.

	Reported name of the Redis instance. This can be in the form
	The reported name of the Redis instance. This can be in the form of

		@@ -0,0 +1,13 @@
		# Redis

		Status: [Experimental](../../document-status.md)

Add semantic conventions for redis receiver #2145

Add semantic conventions for redis receiver #2145

Conversation

manang-splunk commented Nov 22, 2021

Changes

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

manang-splunk Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manang-splunk Dec 3, 2021 • edited Loading

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 Dec 2, 2021

hprajapati-splunk commented Dec 14, 2021

tigrannajaryan commented Dec 15, 2021

manang-splunk commented Dec 16, 2021

tigrannajaryan commented Dec 20, 2021

github-actions bot commented Dec 28, 2021

github-actions bot commented Jan 11, 2022

tigrannajaryan commented Jan 12, 2022

manang-splunk commented Jan 13, 2022

github-actions bot commented Jan 21, 2022

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Jan 21, 2022

joaopgrassi left a comment

Choose a reason for hiding this comment

joaopgrassi Jan 22, 2022 • edited Loading

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

joaopgrassi commented Jan 24, 2022

github-actions bot commented Feb 8, 2022

github-actions bot commented Feb 15, 2022

manang-splunk Nov 26, 2021 •

edited

Loading

manang-splunk Dec 3, 2021 •

edited

Loading

joaopgrassi Jan 22, 2022 •

edited

Loading