Redis instrumentation breaks when host value is not present in the connection kwargs

[eexwhyzee]

Recently upgraded from 0.11b0 to 0.19b0 and having issues getting the redis instrumentation to work with the fakeredis package that is being currently used to mock redis for testing.

It appears that the host value in the connection kwargs is not preserved in the fakeredis implementation (they explicitly remove it in their code) and is causing the tests to break.

Previously the tests were passing fine with 0.11b0 since it looks like the connection info is just omitted if they were not found.

Was wondering if _extract_conn_attributes can be a little bit more forgiving if the values aren't found? For instance, i was able to get the tests passing after i made slight changes to the _extract_conn_attributes function to have a default value if the host key was not found.

Thanks for any help in advance, and also willing to submit a PR if needed!

Describe your environment

• python 3.6.9
• opentelemetry-instrumentation-redis==0.19b0
• fakeredis==1.5.0

Steps to reproduce
failing test:

conn_kwargs = {'db': 0, 'decode_responses': False, 'encoding': 'utf-8', 'encoding_errors': 'strict', ...}

    def _extract_conn_attributes(conn_kwargs):
        """ Transform redis conn info into dict """
        attributes = {
            "db.system": "redis",
        }
        db = conn_kwargs.get("db", 0)
        attributes["db.name"] = db
        attributes["db.redis.database_index"] = db
        try:
>           attributes["net.peer.name"] = conn_kwargs["host"]
E           KeyError: 'host'

conn_kwargs = {'db': 0, 'decode_responses': False, 'encoding': 'utf-8', 'encoding_errors': 'strict', ...}

    def _extract_conn_attributes(conn_kwargs):
        """ Transform redis conn info into dict """
        attributes = {
            "db.system": "redis",
        }
        db = conn_kwargs.get("db", 0)
        attributes["db.name"] = db
        attributes["db.redis.database_index"] = db
        try:
            attributes["net.peer.name"] = conn_kwargs["host"]
            attributes["net.peer.port"] = conn_kwargs.get("port", 6379)
            attributes["net.transport"] = "IP.TCP"
        except KeyError:
>           attributes["net.peer.name"] = conn_kwargs["path"]
E           KeyError: 'path'

redis connection kwargs vs fakeredis connection kwargs:

In [1]: import redis                                                                                     

In [2]: import fakeredis                                                                                 

In [3]: r_conn = redis.Redis(host='localhost', port=6379, db=0)                                          

In [4]: r_conn.connection_pool.connection_kwargs["host"]                                                 
Out[4]: 'localhost'

In [5]: faker_conn = fakeredis.FakeStrictRedis(host='localhost', port=6379, db=0)                        

In [6]: faker_conn.connection_pool.connection_kwargs["host"]                                             
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-6-48d7e1f1cda0> in <module>
----> 1 faker_conn.connection_pool.connection_kwargs["host"]

KeyError: 'host'

What is the expected behavior?
What did you expect to see?

What is the actual behavior?
What did you see instead?

Additional context
Add any other context about the problem here.

[owais]

@eexwhyzee Thanks for reporting. Feel free to send a PR that extracts the host attribute defensively.

[srikanthccv]

It appears that the host value in the connection kwargs is not preserved in the fakeredis implementation (they explicitly remove it in their code) and is causing the tests to break.

Why do they do it? We certainly don't want instrumentations to break when something is missing but I was wondering Is it possible to not have both (host, port) and (path) in connection kwargs?

[eexwhyzee]

@eexwhyzee Thanks for reporting. Feel free to send a PR that extracts the host attribute defensively.

cool cool, i can try to get a PR out sometime this week

It appears that the host value in the connection kwargs is not preserved in the fakeredis implementation (they explicitly remove it in their code) and is causing the tests to break.

Why do they do it? We certainly don't want instrumentations to break when something is missing but I was wondering Is it possible to not have both (host, port) and (path) in connection kwargs?

I haven't asked the folks over at fakeredis yet for more context, but I figured to start a convo here first since the previous behavior in 0.11b0 was just to pass if these values were not present

[srikanthccv]

Instrumentations keep getting updated to make sure they follow semantic conventions(until they become stable) and database sem conv suggest they should have at least one of net.peer.name , net.peer.ip set. I think that was reason behaviour changed since 0.11b0. At the same time instrumentations should never throw exceptions and break regular user flow. Apparently there are also some other issues with Redis instrumentation as highlighted in #265 (Author is not active) and #304. Feel free to make any changes that improves the instrumentation as well.

[eexwhyzee]

Instrumentations keep getting updated to make sure they follow semantic conventions(until they become stable) and database sem conv suggest they should have at least one of net.peer.name , net.peer.ip set. I think that was reason behaviour changed since 0.11b0. At the same time instrumentations should never throw exceptions and break regular user flow. Apparently there are also some other issues with Redis instrumentation as highlighted in #265 (Author is not active) and #304. Feel free to make any changes that improves the instrumentation as well.

cool cool, thanks for the additional context!

My initial thought for changes were along the lines of:

def _extract_conn_attributes(conn_kwargs):
    """ Transform redis conn info into dict """
    attributes = {
        "db.system": "redis",
    }
    db = conn_kwargs.get("db", 0)
    attributes["db.name"] = db
    attributes["db.redis.database_index"] = db

    if "path" in conn_kwargs:
        attributes["net.peer.name"] = conn_kwargs["path"]
        attributes["net.transport"] = "Unix"
    else:
        attributes["net.peer.name"] = conn_kwargs.get("host", "localhost")
        attributes["net.peer.port"] = conn_kwargs.get("port", 6379)
        attributes["net.transport"] = "IP.TCP"

    return attributes

however for #304, seems like its a redis sentinel specific thing where it does not include either host or path in the connection kwargs? Using localhost as a default host value made sense for my fakeredis use case since it is local testing, but might not make total sense in that specific redis sentinel case (maybe a more generic default value like redis?). I'm not very familiar with sentinel so any additional input here would be appreciated

[github-actions]

This issue was marked stale due to lack of activity. It will be closed in 30 days.