Skip to content
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

Merged

Conversation

arielvalentin
Copy link
Contributor

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

docs/database/mysql.md Outdated Show resolved Hide resolved
docs/database/mysql.md Outdated Show resolved Hide resolved
@arielvalentin
Copy link
Contributor Author

@pyohannes @AlexanderWert Is there something more that you need from me? Are you wanting to explore the route of generic attribute naming?

@Oberon00
Copy link
Member

Oberon00 commented Oct 1, 2023

As I understand the new server/client.address conventions

The hostname of the MySQL server instance bound to the current connection. This is useful in cases where the client is connecting to a MySQL server via a proxy or in a clustered environment

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.

@pyohannes
Copy link
Contributor

@Oberon00

This should be filled in in server.address. The current definition of server.address might need to be / have already implicitly changed. See #342.

I had the same thought, but there's more to it: #345 (comment):

Got it, so db.mysql.instance.address will identify the node inside the MySQL cluster. And server.address will be the address the user connects to.

@arielvalentin
Copy link
Contributor Author

cc: @cschleiden @andyedison @adrianna-chang-shopify @open-telemetry/ruby-contrib-approvers

Copy link
Member

@AlexanderWert AlexanderWert left a 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).

model/trace/database.yaml Outdated Show resolved Hide resolved
@arielvalentin
Copy link
Contributor Author

@AlexanderWert I have rebased and resolved conflicts as well as renamed the attribute to db.instance.id.

Copy link
Member

@AlexanderWert AlexanderWert left a 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.)

docs/database/database-spans.md Outdated Show resolved Hide resolved
model/trace/database.yaml Outdated Show resolved Hide resolved
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.
@arielvalentin
Copy link
Contributor Author

@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?

@AlexanderWert
Copy link
Member

@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.

@AlexanderWert AlexanderWert merged commit efd9345 into open-telemetry:main Nov 30, 2023
9 checks passed
@AlexanderWert AlexanderWert changed the title feat: Add MySQL instance address attribute feat: Add db.instance.id attribute Nov 30, 2023
AlexanderWert added a commit to AlexanderWert/semantic-conventions that referenced this pull request Nov 30, 2023
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@arielvalentin arielvalentin deleted the add-mysql-instance-address branch November 30, 2023 14:42
arminru added a commit that referenced this pull request Nov 30, 2023
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Co-authored-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants