-
Notifications
You must be signed in to change notification settings - Fork 599
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.
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
Remove db.name attribute from Redis instrumentation #1427
Remove db.name attribute from Redis instrumentation #1427
Conversation
… name concept in Redis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the test case for this change
I've tried to fix docker-tests which is failing but it looks there is more generic issue... I've run it locally on my machine and got an error related to psycopg2 library:
To be 100% sure that issue is not related to the fix I've made, I've cloned main repo and run docker-tests and got same error message... To my mind it looks like some issue with packages dependencies. Has anyone experienced similar issue recently? |
That's a regular python dep installation issue. It is probably not related to this project, but in general, you may have an issue installing the above. Are you sure your system is set up to run the psycopg2? |
Hi. I've added another commit where I've simply removed verification of db.name attribute from test functions. @sanketmehta28 since we're removing db.name attribute from instrumentation, do we really need another test function for that or removing verification of that attribute from existing functions would be enough? |
removing the verification of the attribute and adding verification for new attribute should be sufficient |
Description
There is no DB name concept in Redis (databases are numbered from 0 to 15), hence sending db.name attribute to OTel collector doesn't make sense. In the current implementation db.name attribute is defaulted to DB number. Technically, it shouldn't be any problem with that but it may break some OpenTelemetry backends which expects a real DB name not a number. There is another attribute db.redis.database_index which should be used for this purpose.
After this fix, we will be also alligned with below document where is stated that for Redis db.name attribute shouldn't be set:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md
Fixes #1395
Type of change
How Has This Been Tested?
Firstly, I run following command to test the setup after the code change:
tox -e test-instrumentation-redis
Then, I tested that locally with some app and after removing db.name attribute from instrumentation, it is not exported to OTel collector as expected.
Before the fix:
After the fix:
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.