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

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

Merged
merged 4 commits into from
Aug 14, 2023

Conversation

adinauer
Copy link
Member

📜 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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2023

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
Member Author

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

github-actions bot commented Aug 11, 2023

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

codecov bot commented Aug 11, 2023

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.

Copy link
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 👍

assertEquals("memory", details.dbName)
}

@Test
Copy link
Collaborator

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
20 of 21 checks passed
@adinauer adinauer deleted the feat/db-system-and-name-span-data branch August 14, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants