-
Notifications
You must be signed in to change notification settings - Fork 181
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
feat: Add db.instance.id attribute #345
feat: Add db.instance.id attribute #345
Conversation
@pyohannes @AlexanderWert Is there something more that you need from me? Are you wanting to explore the route of generic attribute naming? |
As I understand the new server/client.address conventions
This should be filled in in server.address. The current definition of server.address might need to be / have already implicitly changed. See #342. CC @trask @lmolkova It seems to me that the new address conventions should make this PR redundant, maybe this request could inform / validate the design of #342. |
I had the same thought, but there's more to it: #345 (comment):
|
cc: @cschleiden @andyedison @adrianna-chang-shopify @open-telemetry/ruby-contrib-approvers |
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.
See my inline comment regarding the name of the generic attribute.
Please, also resolve the conflicts and update this PR as some of the previous fields have changed (e.g. server.socket.address
).
7fd0784
to
9876d28
Compare
@AlexanderWert I have rebased and resolved conflicts as well as renamed the attribute to |
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.
Nit: a small suggestion on the wording to make clear this field can contain any type of identifier (address, unique name, ids, etc.)
a4ef6f4
to
dbc3e1e
Compare
At GitHub we run MySQL in multi-node clusters and are often interested in knowing the hostname of a node that is executing a query or command. In order to acheive this we added an attribute to the Trilogy instrumentation named `db.mysql.instance.address` which is populated with the hostname of the node: <open-telemetry/opentelemetry-ruby-contrib#487> This change adds the attribute to the database model as a MySQL specific connection attribute.
dbc3e1e
to
4edbffe
Compare
@AlexanderWert thanks again for the review I have rebased and fixed the latest conflicts introduced by the registry refactoring. Is there anything else that I need to do? |
LGTM! We just need more approvals. |
Signed-off-by: Alexander Wert <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Alexander Wert <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Fixes # N/A
Changes
At GitHub we run MySQL in multi-node clusters and are often interested in knowing the hostname of a node that is executing a query or command.
In order to acheive this we added an attribute to the Trilogy instrumentation named
db.mysql.instance.address
which is populated with the hostname of the node:open-telemetry/opentelemetry-ruby-contrib#487
This change adds the attribute to the database model as a MySQL specific connection attribute.
Merge requirement checklist