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.

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

Update pymemcache instrumentation to follow semantic conventions #183

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Update pymemcache instrumentation to follow semantic conventions #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eba288a
Update pymemcache to follow semantic conventions
srikanthccv Nov 14, 2020
8dab100
Move _TRANSPORT under n/w attrs
srikanthccv Nov 15, 2020
0fc6fa2
Add CHANGELOG entry
srikanthccv Nov 15, 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
3 changes: 3 additions & 0 deletions instrumentation/opentelemetry-instrumentation-pymemcache/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

- Update pymemcache instrumentation to follow semantic conventions
([#183](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/183))

## Version 0.13b0

Released 2020-09-17
Expand Down
12 changes: 6 additions & 6 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,14 +55,13 @@
# 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 = "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"
_CMD = "memcached.command"
COMMANDS = [
"set",
"set_many",
Expand Down Expand Up @@ -115,7 +114,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={}
) as span:
Copy link
Contributor

Choose a reason for hiding this comment

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

From before, does pymemcache not have the concept of name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if I understood you correctly, but memcache supports this list of commands. We are using the command as a span name.

Copy link
Contributor

@lzchen lzchen Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I was referring to adding the db.name attribute. We should have it as a span attribute and also use it as a span name fallback if the command isn't present for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't have any concept of databases that can be used for db.name.

try:
if span.is_recording():
Expand Down Expand Up @@ -173,9 +172,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] = "IP.TCP"
elif isinstance(instance.server, str):
address_attributes[_URL] = "memcached://{}".format(instance.server)
address_attributes[_HOST] = instance.server
address_attributes[_TRANSPORT] = "Unix"

return address_attributes

Expand Down
36 changes: 12 additions & 24 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 @@ -62,17 +62,12 @@ def check_spans(self, spans, num_expected, queries_expected):
self.assertEqual(num_expected, len(spans))

for span, query in zip(spans, queries_expected):
self.assertEqual(span.name, "memcached.command")
self.assertIs(span.kind, trace_api.SpanKind.INTERNAL)
self.assertEqual(
span.attributes["net.peer.name"], "{}".format(TEST_HOST)
)
command, *_ = query.split(" ")
self.assertEqual(span.name, command)
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(span.attributes["net.peer.name"], 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 @@ -214,10 +209,8 @@ def test_set_get(self):
spans = self.memory_exporter.get_finished_spans()

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

def test_append_stored(self):
client = self.make_client([b"STORED\r\n"])
Expand Down Expand Up @@ -517,17 +510,12 @@ def check_spans(self, spans, num_expected, queries_expected):
self.assertEqual(num_expected, len(spans))

for span, query in zip(spans, queries_expected):
self.assertEqual(span.name, "memcached.command")
self.assertIs(span.kind, trace_api.SpanKind.INTERNAL)
self.assertEqual(
span.attributes["net.peer.name"], "{}".format(TEST_HOST)
)
command, *_ = query.split(" ")
self.assertEqual(span.name, command)
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)
self.assertEqual(span.attributes["net.peer.name"], 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