Update database instrumentations to follow semantic conventions #175

srikanthccv · 2020-11-10T14:42:18Z

Addresses #159

toumorokoshi

code looks good! could you add changelog entries for each the instrumentations? and we should be good to go.

srikanthccv · 2020-11-11T05:09:25Z

@toumorokoshi I will add the entries. General question for my reference so that I don't miss it in future, What kind of changes require a CHANGELOG entry? Is it all or is it some based on the chage? Thanks !

toumorokoshi · 2020-11-11T05:20:26Z

What kind of changes require a CHANGELOG entry?

Pretty much every change that affects user behavior. Something like span attributes changing could be pretty impactful since people author queries and aggregations based on values in specific attributes.

lzchen · 2020-11-11T14:56:25Z

...elemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py

@@ -115,7 +115,7 @@ def wrapper(wrapped, instance, args, kwargs):
 @_with_tracer_wrapper
 def _wrap_cmd(tracer, cmd, wrapped, instance, args, kwargs):
    with tracer.start_as_current_span(
-        _CMD, kind=SpanKind.INTERNAL, attributes={}
+        _CMD, kind=SpanKind.CLIENT, attributes={}


I believe the span name should be a low-cardinality value representing the executed statement.

I would also add db.name as a span attribute to be consistent with the other instrumentations.

The span name for all pymemcache commands is memcached.command. Regarding db.name there is db.system which is set to here. Do you think we need db.name set to same as well?

The span name for all pymemcache commands is memcached.command

I'm not too familiar in how to use pymemcache, but does that mean there is no "command" or "statement" that is executed?

Do you think we need db.name set to same as well?

Yes, we have db.name in other instrumentations so we should be consistent.

It has commands. Yes we should be using them instead of hard-coded memcached.command for span name. I don't think db.name is applicable for memcached.

lzchen · 2020-11-11T14:58:56Z

.../opentelemetry-instrumentation-pymongo/src/opentelemetry/instrumentation/pymongo/__init__.py

@@ -73,21 +73,23 @@ def started(self, event: monitoring.CommandStartedEvent):
            span = self._tracer.start_span(name, kind=SpanKind.CLIENT)


In terms of the name, I think we can remove the "DATABASE_TYPE" portion and just use the command_name.

EDIT: We can leave these in until the specs are finalized.

lzchen · 2020-11-11T15:03:05Z

.../opentelemetry-instrumentation-pymongo/src/opentelemetry/instrumentation/pymongo/__init__.py

                span.set_attribute("db.statement", statement)
                if event.connection_id is not None:
                    span.set_attribute("net.peer.name", event.connection_id[0])
                    span.set_attribute("net.peer.port", event.connection_id[1])

                # pymongo specific, not specified by spec
-                span.set_attribute("db.mongo.operation_id", event.operation_id)
-                span.set_attribute("db.mongo.request_id", event.request_id)
+                span.set_attribute(


I would remove these attributes as they are not in the semantic conventions.

@toumorokoshi also expressed the same concern on this attrs. I will remove them.

lzchen · 2020-11-11T15:04:09Z

.../opentelemetry-instrumentation-pymongo/src/opentelemetry/instrumentation/pymongo/__init__.py

@@ -106,7 +108,7 @@ def succeeded(self, event: monitoring.CommandSucceededEvent):
            return
        if span.is_recording():
            span.set_attribute(
-                "db.mongo.duration_micros", event.duration_micros
+                "db.mongodb.duration_micros", event.duration_micros


And these should be removed as well.

The spec doesn't have these either.

lzchen · 2020-11-11T15:09:13Z

...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py

@@ -20,15 +20,14 @@
 def _extract_conn_attributes(conn_kwargs):
    """ Transform redis conn info into dict """
    attributes = {
-        "db.type": "redis",


Span name for redis needs to be fixed too.

This is span attribute. The span name for redis is redis.command.

Yes, I only highlighted this because Github didn't let point to the above code.

Simiarly to pymemcache for redis, is there not a concept of "statement" or "command"? The string "redis.command" doesn't really tell me much in terms of what logical operation redis is doing.

The string "redis.command" doesn't really tell me much in terms of what logical operation redis is doing.

Agree.

lzchen · 2020-11-11T15:13:10Z

I think there are a couple of other changes that are missing, from dbapi and asyncpg specifically.
db.system, db.name should be span attributes. The span name should be the command executed. If available provide the attributes related to connections as well.

lzchen

Some comments.

srikanthccv · 2020-11-11T16:29:40Z

db.system, db.name should be span attributes.

Spec has db.system I haven't seen db.name mentioned.

The span name should be the command executed.

I would appreciate some more elaboration on this.

If available provide the attributes related to connections as well.

redis wasn't setting the connection attributes net.peer.name and net.peer.port as mentioned in spec. I have added them in this PR. I only came across redis not doing that I will check others also.

lzchen · 2020-11-12T14:19:36Z

@lonewolf3739

Spec has db.system I haven't seen db.name mentioned.

db.name is mentioned under span name in the specs, in which it should be the span name if the "command executed" is not present.

I would appreciate some more elaboration on this.

I expect the name to be the "command executed" on the database. For example, commands for mongobdb.

srikanthccv · 2020-11-14T18:56:26Z

Hi @lzchen ,

I am thinking of closing this PR in favor of opening multiple small PRs as changes are becoming large. What do you suggest?

srikanthccv · 2020-11-15T13:01:22Z

This is becoming huge. I will be opening multiple small PRs which will be easier to work and review.

srikanthccv added 4 commits November 10, 2020 20:06

Update redis instrumentation to follow semantic conventions

90aed52

Update pymongo instrumentation to follow semantic conventions

ab76f56

Update pymemcache instrumentation to follow semantic conventions

e19b3ba

Remove url from attributes

a9070ab

srikanthccv requested review from a team, toumorokoshi and aabmass and removed request for a team November 10, 2020 14:42

toumorokoshi suggested changes Nov 11, 2020

View reviewed changes

toumorokoshi mentioned this pull request Nov 11, 2020

documentation on what entries require a changelog #179

Merged

1 task

Add CHANGELOG entries

99d6dfc

lzchen reviewed Nov 11, 2020

View reviewed changes

lzchen requested changes Nov 11, 2020

View reviewed changes

Merge branch 'master' into db-semantic-conv

793cbc2

srikanthccv closed this Nov 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update database instrumentations to follow semantic conventions #175

Update database instrumentations to follow semantic conventions #175

srikanthccv commented Nov 10, 2020

toumorokoshi left a comment

srikanthccv commented Nov 11, 2020

toumorokoshi commented Nov 11, 2020

lzchen Nov 11, 2020

lzchen Nov 11, 2020

srikanthccv Nov 11, 2020

lzchen Nov 12, 2020

srikanthccv Nov 13, 2020

lzchen Nov 11, 2020

lzchen Nov 11, 2020

lzchen Nov 11, 2020

srikanthccv Nov 11, 2020

lzchen Nov 11, 2020

srikanthccv Nov 11, 2020

lzchen Nov 11, 2020

srikanthccv Nov 11, 2020

lzchen Nov 12, 2020

srikanthccv Nov 13, 2020

lzchen commented Nov 11, 2020

lzchen left a comment

srikanthccv commented Nov 11, 2020

lzchen commented Nov 12, 2020

srikanthccv commented Nov 14, 2020

srikanthccv commented Nov 15, 2020

		@@ -73,21 +73,23 @@ def started(self, event: monitoring.CommandStartedEvent):
		span = self._tracer.start_span(name, kind=SpanKind.CLIENT)

Update database instrumentations to follow semantic conventions #175

Update database instrumentations to follow semantic conventions #175

Conversation

srikanthccv commented Nov 10, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

srikanthccv commented Nov 11, 2020

toumorokoshi commented Nov 11, 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

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

lzchen commented Nov 11, 2020

lzchen left a comment

Choose a reason for hiding this comment

srikanthccv commented Nov 11, 2020

lzchen commented Nov 12, 2020

srikanthccv commented Nov 14, 2020

srikanthccv commented Nov 15, 2020