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

Jump to bottom

Update redis instrumentation to follow semantic conventions #184

Merged
merged 11 commits into from
Nov 19, 2020
Merged

Update redis instrumentation to follow semantic conventions #184

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ee90c7e
Update redis instrumentation to follow semantic conventions
srikanthccv Nov 14, 2020
6f2d60c
Fix docker tests
srikanthccv Nov 14, 2020
edaf0b1
Add CHANGELOG entry
srikanthccv Nov 16, 2020
857581c
Remove PIPELINE from span name
srikanthccv Nov 16, 2020
e7a121f
Merge branch 'master' into redis-semantic-conv
lzchen Nov 17, 2020
2f1e90b
Merge branch 'master' into redis-semantic-conv
lzchen Nov 17, 2020
ea6ed5c
Use db as fallback for span name
srikanthccv Nov 17, 2020
8fcf6fe
Merge branch 'master' into redis-semantic-conv
lzchen Nov 17, 2020
772aaa4
Merge branch 'master' into redis-semantic-conv
Nov 18, 2020
5224a96
Update CHANGELOG entry
srikanthccv Nov 19, 2020
7716388
Merge branch 'redis-semantic-conv' of https://github.com/lonewolf3739…
srikanthccv Nov 19, 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
2 changes: 2 additions & 0 deletions instrumentation/opentelemetry-instrumentation-redis/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

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

## Version 0.13b0

Released 2020-09-17
Expand Down
7 changes: 4 additions & 3 deletions ...n/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@

_DEFAULT_SERVICE = "redis"
_RAWCMD = "db.statement"
_CMD = "redis.command"


def _set_connection_attributes(span, conn):
Expand All @@ -75,7 +74,7 @@ def _traced_execute_command(func, instance, args, kwargs):
tracer = getattr(redis, "_opentelemetry_tracer")
query = _format_command_args(args)
with tracer.start_as_current_span(
_CMD, kind=trace.SpanKind.CLIENT
args[0], kind=trace.SpanKind.CLIENT
) as span:
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
if span.is_recording():
span.set_attribute("service", tracer.instrumentation_info.name)
Expand All @@ -91,8 +90,10 @@ def _traced_execute_pipeline(func, instance, args, kwargs):
cmds = [_format_command_args(c) for c, _ in instance.command_stack]
resource = "\n".join(cmds)

span_name = " ".join([args[0] for args, _ in instance.command_stack])

with tracer.start_as_current_span(
_CMD, kind=trace.SpanKind.CLIENT
span_name, kind=trace.SpanKind.CLIENT
) as span:
if span.is_recording():
span.set_attribute("service", tracer.instrumentation_info.name)
Expand Down
13 changes: 7 additions & 6 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,16 @@
def _extract_conn_attributes(conn_kwargs):
""" Transform redis conn info into dict """
attributes = {
"db.type": "redis",
"db.instance": conn_kwargs.get("db", 0),
"db.system": "redis",
"db.name": conn_kwargs.get("db", 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the footnote on the OTel Database Specificatiosn seems says it is required to use db.redis.database_index if the connection is not 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, maybe create a new issue to address this? @lonewolf3739

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do that.

}
try:
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this is old code, but an if/else statement checking existence of keys is faster than a try/catch (more work like capture the stack when an exception is thrown)

Copy link
Member Author

Choose a reason for hiding this comment

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

https://docs.python.org/3/glossary.html?highlight=eafp#term-eafp, this is the reason I let this be try.. catch. I will change it if needed.

attributes["db.url"] = "redis://{}:{}".format(
conn_kwargs["host"], conn_kwargs["port"]
)
attributes["net.peer.name"] = conn_kwargs["host"]
attributes["net.peer.ip"] = conn_kwargs["port"]
attributes["net.transport"] = "IP.TCP"
except KeyError:
pass # don't include url attribute
attributes["net.peer.name"] = conn_kwargs["path"]
attributes["net.transport"] = "Unix"

return attributes

Expand Down
2 changes: 1 addition & 1 deletion instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def test_span_properties(self):
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
self.assertEqual(span.name, "redis.command")
self.assertEqual(span.name, "GET")
self.assertEqual(span.kind, SpanKind.CLIENT)

def test_not_recording(self):
Expand Down
21 changes: 10 additions & 11 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 @@ -33,22 +33,21 @@ def tearDown(self):
super().tearDown()
RedisInstrumentor().uninstrument()

def _check_span(self, span):
def _check_span(self, span, name):
self.assertEqual(span.attributes["service"], self.test_service)
self.assertEqual(span.name, "redis.command")
self.assertEqual(span.name, name)
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["net.peer.name"], "localhost")
self.assertEqual(span.attributes["net.peer.ip"], 6379)

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

spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
self._check_span(span)
self._check_span(span, "MGET")
self.assertTrue(
span.attributes.get("db.statement").startswith("MGET 0 1 2 3")
)
Expand All @@ -59,7 +58,7 @@ def test_basics(self):
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
self._check_span(span)
self._check_span(span, "GET")
self.assertEqual(span.attributes.get("db.statement"), "GET cheese")
self.assertEqual(span.attributes.get("redis.args_length"), 2)

Expand All @@ -73,7 +72,7 @@ def test_pipeline_traced(self):
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
self._check_span(span)
self._check_span(span, "SET RPUSH HGETALL")
self.assertEqual(
span.attributes.get("db.statement"),
"SET blah 32\nRPUSH foo éé\nHGETALL xxx",
Expand All @@ -91,7 +90,7 @@ def test_pipeline_immediate(self):
# single span for the whole pipeline
self.assertEqual(len(spans), 2)
span = spans[0]
self._check_span(span)
self._check_span(span, "SET")
self.assertEqual(span.attributes.get("db.statement"), "SET b 2")

def test_parent(self):
Expand All @@ -115,4 +114,4 @@ def test_parent(self):
self.assertEqual(
child_span.attributes.get("service"), self.test_service
)
self.assertEqual(child_span.name, "redis.command")
self.assertEqual(child_span.name, "GET")