Skip to content

Send db.system and db.name in span data#2894

Merged
adinauer merged 4 commits intomainfrom
feat/db-system-and-name-span-data
Aug 14, 2023
Merged

Send db.system and db.name in span data#2894
adinauer merged 4 commits intomainfrom
feat/db-system-and-name-span-data

Conversation

@adinauer
Copy link
Copy Markdown
Member

@adinauer adinauer commented Aug 11, 2023

📜 Description

Send db.system and db.name in DB span data. To do this we have to parse the JDBC connection string.

💡 Motivation and Context

Parts of what's required for #2893

💚 How did you test it?

Unit tests, manually

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • More fields could be extracted from the JDBC string
  • Not all db systems are fully supported by the parser yet

@adinauer adinauer requested review from AbhiPrasad and lbloder August 11, 2023 09:26
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 11, 2023 •
edited
Loading

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 50fc307

@adinauer
Copy link
Copy Markdown
Member Author

adinauer commented Aug 11, 2023

@AbhiPrasad I only parsed db.system and db.name for now. We can come back to this and improve in the future. db.system should be parsed for most db systems but db.name is only supported for some (see tests) at the moment.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 11, 2023 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 331.58 ms 391.98 ms 60.40 ms
Size 1.72 MiB 2.29 MiB 575.74 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4bf202b 364.28 ms 419.66 ms 55.38 ms
3baa73f 267.45 ms 388.30 ms 120.85 ms
f274c79 334.86 ms 348.54 ms 13.68 ms
496bdfd 272.86 ms 407.33 ms 134.48 ms
fe10f05 314.71 ms 360.62 ms 45.90 ms
caf50ec 302.36 ms 325.25 ms 22.89 ms
87b3774 310.48 ms 362.04 ms 51.56 ms
fb296f0 340.61 ms 394.88 ms 54.27 ms
f60279b 324.60 ms 345.33 ms 20.73 ms
8820c5c 330.60 ms 416.86 ms 86.26 ms

App size

Revision Plain With Sentry Diff
4bf202b 1.72 MiB 2.29 MiB 575.54 KiB
3baa73f 1.72 MiB 2.29 MiB 575.52 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
496bdfd 1.72 MiB 2.28 MiB 571.82 KiB
fe10f05 1.72 MiB 2.29 MiB 575.54 KiB
caf50ec 1.72 MiB 2.29 MiB 575.24 KiB
87b3774 1.72 MiB 2.29 MiB 575.54 KiB
fb296f0 1.72 MiB 2.29 MiB 575.70 KiB
f60279b 1.72 MiB 2.29 MiB 575.23 KiB
8820c5c 1.72 MiB 2.28 MiB 571.82 KiB

Previous results on branch: feat/db-system-and-name-span-data

Startup times

Revision Plain With Sentry Diff
25a667e 344.12 ms 399.22 ms 55.10 ms
78d13e7 327.33 ms 373.72 ms 46.39 ms

App size

Revision Plain With Sentry Diff
25a667e 1.72 MiB 2.29 MiB 575.74 KiB
78d13e7 1.72 MiB 2.29 MiB 575.74 KiB

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 11, 2023 •
edited
Loading

Codecov Report

Patch coverage: 83.92% and project coverage change: -0.92% ⚠️

Comparison is base (549cbb4) 80.52% compared to head (50fc307) 79.61%.
Report is 2 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2894      +/-   ##
============================================
- Coverage     80.52%   79.61%   -0.92%     
- Complexity     4714     6038    +1324     
============================================
  Files           371      469      +98     
  Lines         17566    23294    +5728     
  Branches       2365     3176     +811     
============================================
+ Hits          14145    18545    +4400     
- Misses         2447     3353     +906     
- Partials        974     1396     +422
Files Changed Coverage Δ
...n/java/io/sentry/jdbc/SentryJdbcEventListener.java 85.71% <81.25%> (-2.75%) ⬇️
...bc/src/main/java/io/sentry/jdbc/DatabaseUtils.java 82.14% <82.14%> (ø)
...ntry/src/main/java/io/sentry/util/StringUtils.java 90.62% <100.00%> (+2.16%) ⬆️

... and 97 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lbloder
lbloder approved these changes Aug 11, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@lbloder lbloder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Comment thread sentry-jdbc/src/test/kotlin/io/sentry/jdbc/DatabaseUtilsTest.kt
assertEquals("memory", details.dbName)
}

@Test
Copy link
Copy Markdown
Collaborator

@lbloder lbloder Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(l) Maybe add the same test for linux?
Won't make a coverage difference, but IMO makes the tests more complete from a user perspective.

    @Test
    fun `detects db system for sqlite linux`() {
        val details = DatabaseUtils.parse("jdbc:sqlite:/home/sqlite/db/some.db")
        assertEquals("sqlite", details.dbSystem)
        assertEquals("/home/sqlite/db/some.db", details.dbName)
    }

@adinauer adinauer merged commit 3fe387a into main Aug 14, 2023
@adinauer adinauer deleted the feat/db-system-and-name-span-data branch August 14, 2023 10:18
@adinauer adinauer mentioned this pull request Aug 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lbloder lbloder lbloder approved these changes

@romtsn romtsn Awaiting requested review from romtsn romtsn is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

@markushi markushi Awaiting requested review from markushi markushi is a code owner

+1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adinauer @lbloder @AbhiPrasad