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

Should db.instance.id replace elastic and mssql specific attributes? #725

Closed
lmolkova opened this issue Feb 9, 2024 · 5 comments
Closed
Assignees

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Feb 9, 2024

db.instance.id string An identifier (address, unique name, or any other identifier) of the database instance that is executing queries or mutations on the current connection. This is useful in cases where the database is running in a clustered environment and the instrumentation is able to record the node executing the query. The client may obtain this value in databases like MySQL using queries like select @@hostname. mysql-e26b99z.example.com Recommended: If different from the server.address

It seems the intention is to record specific instance host name or IP and network.peer.address should do the job.

After reading more on other this, I wonder if we should update the description to remove any pointers to network/hostnames and simply say that it's a friendly name identifying db instance within a cluster (and it's unrelated to network address/port which are dynamic).

Also, we should remove similar mssql and elastic attributes.

Related: #690

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 9, 2024

Related: #345

After reading the discussion on the PR, it seems server.address was considered, but not the network.peer.address.

But also, #727 and #728 show that it's not an IP address, but a user-friendly name within the cluster (FQDN or unique within a cluster).

We removed network.peer.name (aka net.peer.sock.name aka unborn proxy.address) attribute which might be useful here, but I'd prefer to keep db.instance.id|name as a unified thing and remove db.mssql.instance_name and db.elasticsearch.node.name.

@arielvalentin @AlexanderWert @estolfo - thoughts?

@lmolkova lmolkova changed the title Can db.instance.id be replaced by network.peer.address ? Should db.instance.id replace elastic and mssql specific attributes? Feb 9, 2024
@Oberon00
Copy link
Member

Oberon00 commented Feb 9, 2024

AFAIK, the MS SQL instance does not match your proposed description of db.instance.id. It seems to be a quite special concept, I wonder if you will find anything equivalent in any other db.
MS SQL instances are not members of a cluster. Depending on which "instance" you connect to, you will have different databases. This instance concept seems to allow running logically distinct DBs on the same hostname, so it's rather the opposite of clustering.

https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/database-engine-instances-sql-server?view=sql-server-ver16

@AlexanderWert
Copy link
Member

We removed network.peer.name (aka net.peer.sock.name aka unborn proxy.address) attribute which might be useful here, but I'd prefer to keep db.instance.id|name as a unified thing and remove db.mssql.instance_name and db.elasticsearch.node.name.

@lmolkova Agree, db.instance.id|name can replace the db.elasticsearch.node.name (and with a corresponding, Elasticsearch specific overwrite of thedescription of the attribute in the ES context it should be clear.)

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 9, 2024

AFAIK, the MS SQL instance does not match your proposed description of db.instance.id. It seems to be a quite special concept, I wonder if you will find anything equivalent in any other db. MS SQL instances are not members of a cluster. Depending on which "instance" you connect to, you will have different databases. This instance concept seems to allow running logically distinct DBs on the same hostname, so it's rather the opposite of clustering.

https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/database-engine-instances-sql-server?view=sql-server-ver16

@Oberon00 thanks for the explanation! If we change db.instance.id to be something like "friendly name identifying db instance within the host name (server.address, network.peer.address, etc)" it'd work, right?

@trask
Copy link
Member

trask commented Apr 26, 2024

Closing, as SQLServer instance name is now incorporated into db.namespace, and Elastic is using db.instance.id since #738 (though that change may now be reverted in #972)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

4 participants