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

[Starfish] Add db attributes to database span's span data #2893

Closed
Tracked by #19
AbhiPrasad opened this issue Aug 9, 2023 · 5 comments · Fixed by #2928
Closed
Tracked by #19

[Starfish] Add db attributes to database span's span data #2893

AbhiPrasad opened this issue Aug 9, 2023 · 5 comments · Fixed by #2928

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Aug 9, 2023

To get an overview of this work across all SDKs, please visit:
https://github.com/orgs/getsentry/projects/135

Parent issue: getsentry/team-sdks#19

For the starfish db module it was determined that we need extra database attributes, specifically about the database connection attributes.

NOTE: this should apply to both android and java db spans

We're going to matching the otel db conventions. Either one of server.address or server.socket.address is required. If server.address is set, then server.port should be set. If server.port is different than server.socket.port, than server.socket.port should also be set. In addition, db.name should be set if possible. db.system should match the list in OpenTelemetry's well known conventions.

The main priority items are db.name and db.system

Attribute Type Description Examples Requirement Level
db.system string An identifier for the database management system (DBMS) product being used. mysql Required
server.address string Name of the database host. example.com Conditionally Required: See alternative attributes below.
server.port int Logical server port number 80; 8080; 443 Conditionally Required: [1]
server.socket.address string Physical server IP address or Unix socket address. 10.5.3.2 See below
server.socket.port int Physical server port. 16456 Recommended: If different than server.port.
db.name string This attribute is used to report the name of the database being accessed. For commands that switch the database, this should be set to the target database (even if the command fails). customers; main Conditionally Required: If applicable.

These values should be set on span.data.

NOTE: For the Mobile SDKs only db.name and db.system is required

Motivation

This is being done for the Sentry's new performance views for databases. See a WIP example of the view below:

image
@adinauer
Copy link
Member

db.system and db.name were added via #2894 as a first step. Other info has to be parsed from JDBC connection string as well.

@romtsn
Copy link
Member

romtsn commented Aug 16, 2023

On Android we should check whether it's possbile:

  • To distinguish between in-memory and sqlite for db.system (if not, we should probably always send sqlite)
  • To retrieve the database name

@AbhiPrasad we also instrument Room (ORM) spans, but they don't have the underlying SQL query as a description in them. Would it make sense to add these attributes to the ORM spans at all? The ORM spans now have span op db - should we perhaps change that to something more specific (db.sql.room) to not be accounted for in starfish?

This is how it looks right now
image

@AbhiPrasad
Copy link
Member Author

We need to make sure the starfish views account for the different types of db spans - we'll take care of that. If we could make them their own special span op that would help quite a bit.

Would it make sense to add these attributes to the ORM spans at all

I think it still makes sense to add db.name and db.system, gives us room to build views for that data going forward.

@romtsn
Copy link
Member

romtsn commented Sep 11, 2023

@AbhiPrasad is it fine to close this issue once the Android PR is merged, or do we wanna keep it for the java-backend missing things (like server.{stuff})? We could also create a follow-up issue to not have this one hanging around forever

@AbhiPrasad
Copy link
Member Author

is it fine to close this issue once the Android PR is merged

Yes that sounds good to me. Creating follow-up issues for the Java server specific stuff sounds like a good idea to me.

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

Successfully merging a pull request may close this issue.

4 participants