Update redis instrumentation to follow semantic conventions #184
Conversation
srikanthccv
requested review from
toumorokoshi and
ocelotl
and removed request for
a team
srikanthccv
changed the title
|
cc @lzchen |
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Show resolved
Hide resolved
| "db.type": "redis", | ||
| "db.instance": conn_kwargs.get("db", 0), | ||
| "db.system": "redis", | ||
| "db.name": conn_kwargs.get("db", 0), | ||
| } | ||
| try: |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
@lonewolf3739 Just curious if there was thought to add |
| "db.type": "redis", | ||
| "db.instance": conn_kwargs.get("db", 0), | ||
| "db.system": "redis", | ||
| "db.name": conn_kwargs.get("db", 0), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good catch, maybe create a new issue to address this? @lonewolf3739
Good catch. I will add this. |
Description
Part of #159
Type of change
How Has This Been Tested?
tox -e test-instrumentation-redis