Skip to content

Update database instrumentations to follow semantic conventions #175

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Update database instrumentations to follow semantic conventions #175

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90aed52
Update redis instrumentation to follow semantic conventions
srikanthccv Nov 3, 2020
ab76f56
Update pymongo instrumentation to follow semantic conventions
srikanthccv Nov 3, 2020
e19b3ba
Update pymemcache instrumentation to follow semantic conventions
srikanthccv Nov 3, 2020
a9070ab
Remove url from attributes
srikanthccv Nov 3, 2020
99d6dfc
Add CHANGELOG entries
srikanthccv Nov 11, 2020
793cbc2
Merge branch 'master' into db-semantic-conv
lzchen Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions ...metry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@
# https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/span-general.md#general-network-connection-attributes
_HOST = "net.peer.name"
_PORT = "net.peer.port"
_TRANSPORT_PROTOCOL = "net.transport"
# Database semantic conventions here:
# https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md
_DB = "db.type"
_URL = "db.url"
_DB = "db.system"

_DEFAULT_SERVICE = "memcached"
_RAWCMD = "db.statement"
Expand Down Expand Up @@ -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={}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

) as span:
try:
if span.is_recording():
Expand Down Expand Up @@ -173,9 +173,10 @@ def _get_address_attributes(instance):
host, port = instance.server
address_attributes[_HOST] = host
address_attributes[_PORT] = port
address_attributes[_URL] = "memcached://{}:{}".format(host, port)
address_attributes[_TRANSPORT_PROTOCOL] = "IP.TCP"
elif isinstance(instance.server, str):
address_attributes[_URL] = "memcached://{}".format(instance.server)
address_attributes[_HOST] = instance.server
address_attributes[_TRANSPORT_PROTOCOL] = "Unix"

return address_attributes

Expand Down
21 changes: 7 additions & 14 deletions instrumentation/opentelemetry-instrumentation-pymemcache/tests/test_pymemcache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,12 @@ def check_spans(self, spans, num_expected, queries_expected):

for span, query in zip(spans, queries_expected):
self.assertEqual(span.name, "memcached.command")
self.assertIs(span.kind, trace_api.SpanKind.INTERNAL)
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(
span.attributes["net.peer.name"], "{}".format(TEST_HOST)
)
self.assertEqual(span.attributes["net.peer.port"], TEST_PORT)
self.assertEqual(span.attributes["db.type"], "memcached")
self.assertEqual(
span.attributes["db.url"],
"memcached://{}:{}".format(TEST_HOST, TEST_PORT),
)
self.assertEqual(span.attributes["db.system"], "memcached")
self.assertEqual(span.attributes["db.statement"], query)

def test_set_success(self):
Expand Down Expand Up @@ -215,9 +211,10 @@ def test_set_get(self):

self.assertEqual(len(spans), 2)
self.assertEqual(
spans[0].attributes["db.url"],
"memcached://{}:{}".format(TEST_HOST, TEST_PORT),
spans[0].attributes["net.peer.name"], "{}".format(TEST_HOST)
)
self.assertEqual(spans[0].attributes["net.peer.port"], TEST_PORT)
self.assertEqual(spans[0].attributes["db.system"], "memcached")

def test_append_stored(self):
client = self.make_client([b"STORED\r\n"])
Expand Down Expand Up @@ -518,16 +515,12 @@ def check_spans(self, spans, num_expected, queries_expected):

for span, query in zip(spans, queries_expected):
self.assertEqual(span.name, "memcached.command")
self.assertIs(span.kind, trace_api.SpanKind.INTERNAL)
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(
span.attributes["net.peer.name"], "{}".format(TEST_HOST)
)
self.assertEqual(span.attributes["net.peer.port"], TEST_PORT)
self.assertEqual(span.attributes["db.type"], "memcached")
self.assertEqual(
span.attributes["db.url"],
"memcached://{}:{}".format(TEST_HOST, TEST_PORT),
)
self.assertEqual(span.attributes["db.system"], "memcached")
self.assertEqual(span.attributes["db.statement"], query)

def test_delete_many_found(self):
Expand Down
16 changes: 9 additions & 7 deletions ...entelemetry-instrumentation-pymongo/src/opentelemetry/instrumentation/pymongo/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,23 @@ def started(self, event: monitoring.CommandStartedEvent):
span = self._tracer.start_span(name, kind=SpanKind.CLIENT)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if span.is_recording():
span.set_attribute("component", DATABASE_TYPE)
span.set_attribute("db.type", DATABASE_TYPE)
span.set_attribute("db.instance", event.database_name)
span.set_attribute("db.system", DATABASE_TYPE)
span.set_attribute("db.name", event.database_name)
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

"db.mongodb.operation_id", event.operation_id
)
span.set_attribute("db.mongodb.request_id", event.request_id)

for attr in COMMAND_ATTRIBUTES:
_attr = event.command.get(attr)
if _attr is not None:
span.set_attribute("db.mongo." + attr, str(_attr))
span.set_attribute("db.mongodb." + attr, str(_attr))

# Add Span to dictionary
self._span_dict[_get_span_dict_key(event)] = span
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

And these should be removed as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec doesn't have these either.

)
span.end()

Expand All @@ -119,7 +121,7 @@ def failed(self, event: monitoring.CommandFailedEvent):
return
if span.is_recording():
span.set_attribute(
"db.mongo.duration_micros", event.duration_micros
"db.mongodb.duration_micros", event.duration_micros
)
span.set_status(Status(StatusCode.ERROR, event.failure))
span.end()
Expand Down
26 changes: 14 additions & 12 deletions instrumentation/opentelemetry-instrumentation-pymongo/tests/test_pymongo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,22 @@ def test_started(self):
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(span.name, "mongodb.command_name.find")
self.assertEqual(span.attributes["component"], "mongodb")
self.assertEqual(span.attributes["db.type"], "mongodb")
self.assertEqual(span.attributes["db.instance"], "database_name")
self.assertEqual(span.attributes["db.system"], "mongodb")
self.assertEqual(span.attributes["db.name"], "database_name")
self.assertEqual(span.attributes["db.statement"], "command_name find")
self.assertEqual(span.attributes["net.peer.name"], "test.com")
self.assertEqual(span.attributes["net.peer.port"], "1234")
self.assertEqual(
span.attributes["db.mongo.operation_id"], "operation_id"
span.attributes["db.mongodb.operation_id"], "operation_id"
)
self.assertEqual(
span.attributes["db.mongo.request_id"], "test_request_id"
span.attributes["db.mongodb.request_id"], "test_request_id"
)

self.assertEqual(span.attributes["db.mongo.filter"], "filter")
self.assertEqual(span.attributes["db.mongo.sort"], "sort")
self.assertEqual(span.attributes["db.mongo.limit"], "limit")
self.assertEqual(span.attributes["db.mongo.pipeline"], "pipeline")
self.assertEqual(span.attributes["db.mongodb.filter"], "filter")
self.assertEqual(span.attributes["db.mongodb.sort"], "sort")
self.assertEqual(span.attributes["db.mongodb.limit"], "limit")
self.assertEqual(span.attributes["db.mongodb.pipeline"], "pipeline")

def test_succeeded(self):
mock_event = MockEvent({})
Expand All @@ -83,7 +83,7 @@ def test_succeeded(self):
self.assertEqual(len(spans_list), 1)
span = spans_list[0]
self.assertEqual(
span.attributes["db.mongo.duration_micros"], "duration_micros"
span.attributes["db.mongodb.duration_micros"], "duration_micros"
)
self.assertIs(
span.status.status_code, trace_api.status.StatusCode.UNSET
Expand Down Expand Up @@ -117,7 +117,7 @@ def test_failed(self):
span = spans_list[0]

self.assertEqual(
span.attributes["db.mongo.duration_micros"], "duration_micros"
span.attributes["db.mongodb.duration_micros"], "duration_micros"
)
self.assertIs(
span.status.status_code, trace_api.status.StatusCode.ERROR,
Expand All @@ -139,12 +139,14 @@ def test_multiple_commands(self):
first_span = spans_list[0]
second_span = spans_list[1]

self.assertEqual(first_span.attributes["db.mongo.request_id"], "first")
self.assertEqual(
first_span.attributes["db.mongodb.request_id"], "first"
)
self.assertIs(
first_span.status.status_code, trace_api.status.StatusCode.UNSET,
)
self.assertEqual(
second_span.attributes["db.mongo.request_id"], "second"
second_span.attributes["db.mongodb.request_id"], "second"
)
self.assertIs(
second_span.status.status_code, trace_api.status.StatusCode.ERROR,
Expand Down
13 changes: 6 additions & 7 deletions ...ation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@
def _extract_conn_attributes(conn_kwargs):
""" Transform redis conn info into dict """
attributes = {
"db.type": "redis",
Copy link
Contributor

Choose a reason for hiding this comment

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

Span name for redis needs to be fixed too.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Agree.

"db.instance": conn_kwargs.get("db", 0),
"db.system": "redis",
"db.name": conn_kwargs.get("db", 0),
}
try:
attributes["db.url"] = "redis://{}:{}".format(
conn_kwargs["host"], conn_kwargs["port"]
)
except KeyError:
pass # don't include url attribute
attributes["net.peer.name"] = conn_kwargs["host"]
attributes["net.peer.port"] = conn_kwargs["port"]
except KeyError: # Connected using unix socket
attributes["net.peer.name"] = conn_kwargs["path"]

return attributes

Expand Down
4 changes: 1 addition & 3 deletions tests/opentelemetry-docker-tests/tests/pymongo/test_pymongo_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ def validate_spans(self):
self.assertIsNotNone(pymongo_span.parent)
self.assertIs(pymongo_span.parent, root_span.get_span_context())
self.assertIs(pymongo_span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(
pymongo_span.attributes["db.instance"], MONGODB_DB_NAME
)
self.assertEqual(pymongo_span.attributes["db.name"], MONGODB_DB_NAME)
self.assertEqual(
pymongo_span.attributes["net.peer.name"], MONGODB_HOST
)
Expand Down
7 changes: 3 additions & 4 deletions tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,9 @@ def _check_span(self, span):
self.assertEqual(span.attributes["service"], self.test_service)
self.assertEqual(span.name, "redis.command")
self.assertIs(span.status.status_code, trace.status.StatusCode.UNSET)
self.assertEqual(span.attributes.get("db.instance"), 0)
self.assertEqual(
span.attributes.get("db.url"), "redis://localhost:6379"
)
self.assertEqual(span.attributes.get("db.name"), 0)
self.assertEqual(span.attributes.get("net.peer.name"), "localhost")
self.assertEqual(span.attributes.get("net.peer.port"), 6379)

def test_long_command(self):
self.redis_client.mget(*range(1000))
Expand Down