Skip to content

feat: Add db.instance.id attribute#345

Merged
AlexanderWert merged 2 commits intoopen-telemetry:mainfrom
arielvalentin:add-mysql-instance-address
Nov 30, 2023
Merged

feat: Add db.instance.id attribute#345
AlexanderWert merged 2 commits intoopen-telemetry:mainfrom
arielvalentin:add-mysql-instance-address

Conversation

@arielvalentin
Copy link
Copy Markdown
Contributor

@arielvalentin arielvalentin commented Sep 26, 2023

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

@arielvalentin arielvalentin requested review from a team September 26, 2023 02:57
pyohannes
pyohannes reviewed Sep 26, 2023
View reviewed changes
Comment thread docs/database/mysql.md Outdated
Comment thread docs/database/mysql.md Outdated
AlexanderWert
AlexanderWert reviewed Sep 26, 2023
View reviewed changes
Comment thread docs/database/mysql.md Outdated
@arielvalentin
Copy link
Copy Markdown
Contributor Author

arielvalentin commented Sep 30, 2023

@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
Copy Markdown
Member

Oberon00 commented Oct 1, 2023 •
edited
Loading

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
Copy Markdown
Contributor

pyohannes commented Oct 2, 2023

@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
Copy Markdown
Contributor Author

arielvalentin commented Oct 3, 2023

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

AlexanderWert
AlexanderWert requested changes Nov 15, 2023
View reviewed changes
Copy link
Copy Markdown
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).

Comment thread model/trace/database.yaml Outdated
@arielvalentin arielvalentin force-pushed the add-mysql-instance-address branch from 7fd0784 to 9876d28 Compare November 15, 2023 13:31
@arielvalentin
Copy link
Copy Markdown
Contributor Author

arielvalentin commented Nov 15, 2023

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

AlexanderWert
AlexanderWert approved these changes Nov 15, 2023
View reviewed changes
Copy link
Copy Markdown
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.)

Comment thread docs/database/database-spans.md Outdated
Comment thread model/trace/database.yaml Outdated
@arielvalentin arielvalentin force-pushed the add-mysql-instance-address branch from a4ef6f4 to dbc3e1e Compare November 19, 2023 19:41
@arielvalentin
feat: Add MySQL instance address attribute
4edbffe 
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 arielvalentin force-pushed the add-mysql-instance-address branch from dbc3e1e to 4edbffe Compare November 19, 2023 19:45
@arielvalentin
Copy link
Copy Markdown
Contributor Author

arielvalentin commented Nov 19, 2023

@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
Copy Markdown
Member

AlexanderWert commented Nov 20, 2023

@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 mentioned this pull request Nov 20, 2023
Closed
@AlexanderWert AlexanderWert merged commit efd9345 into open-telemetry:main Nov 30, 2023
@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
@AlexanderWert
Add changelog entry for open-telemetry#345
fb14501 
Signed-off-by: Alexander Wert <[email protected]>
@arielvalentin arielvalentin deleted the add-mysql-instance-address branch November 30, 2023 14:42
@arielvalentin arielvalentin mentioned this pull request Nov 30, 2023
Closed
@trask trask mentioned this pull request Nov 30, 2023
Closed
arminru added a commit that referenced this pull request Nov 30, 2023
@AlexanderWert @arminru
Add changelog entry for #345 (#567)
ed7d3e0 
Co-authored-by: Armin Ruech <[email protected]>
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
@arielvalentin @AlexanderWert @pyohannes
feat: Add MySQL instance address attribute (open-telemetry#345)
f4c16e8 
Co-authored-by: Alexander Wert <[email protected]>
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
@AlexanderWert @arminru @pyohannes
Add changelog entry for open-telemetry#345 (open-telemetry#567)
4255d8a 
Co-authored-by: Armin Ruech <[email protected]>
@lmolkova lmolkova mentioned this pull request Feb 9, 2024
Closed
@mathias mathias mentioned this pull request Mar 29, 2024
Closed
@arielvalentin arielvalentin mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexanderWert AlexanderWert AlexanderWert approved these changes

@jsuereth jsuereth jsuereth approved these changes

@pyohannes pyohannes pyohannes approved these changes

Assignees

@jsuereth jsuereth

Labels

None yet

Projects

Semantic Conventions Triage
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arielvalentin @Oberon00 @pyohannes @AlexanderWert @jsuereth