Draft: Initial get_connection wrapper for redis metrics closes #1148 #1194
Conversation
dbgoytia
force-pushed
the
1040-redis-instrumentation
branch
2 times, most recently
from
3639dc4 to
ec35a24
Compare
dbgoytia
changed the title
...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
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py
Outdated
Show resolved
Hide resolved
dbgoytia
force-pushed
the
1040-redis-instrumentation
branch
3 times, most recently
from
97d31b6 to
9fcbe15
Compare
dbgoytia
force-pushed
the
1040-redis-instrumentation
branch
from
9fcbe15 to
113730d
Compare
| connections_usage.add( | ||
| 1, | ||
| { | ||
| "db.client.connection.usage.state": "used", | ||
| "db.client.connection.usage.name": instance.connection_pool.pid, | ||
| }) | ||
| if callable(response_hook): | ||
| response_hook(span, instance, response) | ||
| finally: | ||
| connections_usage.add( | ||
| -1, | ||
| { | ||
| "db.client.connection.usage.state": "idle", | ||
| "db.client.connection.usage.name": instance.connection_pool.pid, | ||
| }) |
There was a problem hiding this comment.
Can you decide the state of connection yourself based on the command execution? This can't be correct right?
| connections_usage.add( | ||
| 1, | ||
| { | ||
| "db.client.connection.usage.state": "used", |
There was a problem hiding this comment.
Is get connection same as used?
| meter_provider | ||
| ) | ||
| connections_usage = meter.create_up_down_counter( | ||
| name="db.client.connection.usage", |
There was a problem hiding this comment.
It's "db.client.connections.usage"
| connections_usage.add( | ||
| 1, | ||
| { | ||
| "db.client.connection.usage.state": "used", |
There was a problem hiding this comment.
according to the spec, the prefix "db.client.connection.usage" should be omitted. look the comment here: open-telemetry/opentelemetry-js-contrib#1220 (comment)
The name of the attribute is just "state"
| 1, | ||
| { | ||
| "db.client.connection.usage.state": "used", | ||
| "db.client.connection.usage.name": instance.connection_pool.pid, |
There was a problem hiding this comment.
according to the spec, there is no attribute named db.client.connection.usage.name. But there is pool.name. see here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/database-metrics.md
You can also see the conversation about pool.name I had here: open-telemetry/opentelemetry-specification#2958
It was specific for mysql in jacascript but still can give an idea about what is pool name.
| response_hook(span, instance, response) | ||
| finally: | ||
| connections_usage.add( | ||
| -1, |
There was a problem hiding this comment.
There is a cycle of using a connection:
- First you create a connection and it becomes part of the pool ( -> should add +1 to 'idle').
- Then you start using it ( -> should add +1 to 'used' and -1 to 'idle').
- Then you release/finish using it ( -> should add -1 to 'used', and +1 to 'idle').
- Finally it is removed from the pool ( -> should add -1 to 'idle')
You should find these events in the redis code and understand when a connection is created, used, removed etc. each of these events should be reflected in the number of 'idle' and/or 'used' connection.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #1148
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.