open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
705 stars 589 forks source link

Make sure redis instrumentation follows semantic conventions #1627

Open srikanthccv opened 1 year ago

cBiscuitSurprise commented 1 year ago

@srikanthccv I'd like to help with these. I'd like to start here with redis, then move on to other if possible.

cBiscuitSurprise commented 1 year ago

Notional Plan:

cBiscuitSurprise commented 1 year ago

Current attributes:

    "attributes": {
        "db.statement": "GET ?",
        "db.system": "redis",
        "db.redis.database_index": 0,
        "net.peer.name": "localhost",
        "net.peer.port": 6379,
        "net.transport": "ip_tcp",
        "db.redis.args_length": 2
    },
cBiscuitSurprise commented 1 year ago

I created the tests, but found the following:

opentelemetry.semconv.trace doesn't have the following attributes:

  1. server.address
  2. server.port
  3. server.socket.address
  4. network.transport (has net.transport with similar values)
  5. network.type

A couple issues that may be relavant (?):

srikanthccv commented 1 year ago

I assigned this to you. Please see https://github.com/open-telemetry/opentelemetry-python/pull/3251 for updating the semconv.

cBiscuitSurprise commented 1 year ago

Thank you! Should this task (#1627) wait until we're aligned on semconv latest (which is currently 1.24.0)?

Just brainstorming: Is it possible to add a layer in opentelemetry.semconv like:

from opentelemetry.semconv import SemConv
semconv = SemConv(version="1.20.0");
semconv.span_attributes.SERVER_ADDRESS

It doesn't completely solve the coupling, but may help break up the PRs. You could simply have one relatively small CR to make the new version available for use, then incrementally update usage in all of the sub-packages in separate PRs. This could allow skipping sub-package updates if say 1.21.0, 1.22.0, etc. come along before all the sub-packages are updated. Then follow up by deprecating unused versions as all the usage is updated.


Edit: I'm just now realizing this library is versioned in lock-step, so nvm.

cBiscuitSurprise commented 1 year ago

@srikanthccv I created a PR for now it's pointing to the depended PR (the 1.20.0 change), but once that's merged I can open a real one on the main project.

Is this approach acceptable? Basically I added tests and then fixed the impl where things weren't quite right.