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

Set required attribute of DB_SYSTEM #181

Merged
merged 16 commits into from
Jul 24, 2023
Merged

Conversation

cedricziel
Copy link
Contributor

@cedricziel cedricziel commented Jul 21, 2023

This change introduces a default value for db.system, to satisfy the opentelemetry specification.

It also tracks a statement <-> PDO mapping based on the object ids so we can annotate the correct attributes on statement spans.

This change introduces a default value for db.system, to satisfy the opentelemetry specification
@cedricziel cedricziel requested a review from a team July 21, 2023 09:02
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #181 (34e7a63) into main (8f69ac2) will increase coverage by 2.14%.
The diff coverage is 58.33%.

❗ Current head 34e7a63 differs from pull request most recent head 63f30b6. Consider uploading reports for the commit 63f30b6 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #181      +/-   ##
============================================
+ Coverage     32.62%   34.76%   +2.14%     
- Complexity      783      791       +8     
============================================
  Files            69       70       +1     
  Lines          2970     3017      +47     
============================================
+ Hits            969     1049      +80     
+ Misses         2001     1968      -33     
Flag Coverage Δ
7.4 46.08% <ø> (ø)
8.0 34.83% <ø> (ø)
8.1 34.87% <ø> (ø)
8.2 34.71% <58.33%> (+2.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Instrumentation/PDO/src/PDOInstrumentation.php 33.33% <29.03%> (+33.33%) ⬆️
...rc/Instrumentation/PDO/src/PDOAttributeTracker.php 89.65% <89.65%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f69ac2...63f30b6. Read the comment docs.

@cedricziel cedricziel requested a review from brettmc July 22, 2023 11:43
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
@cedricziel cedricziel requested a review from lstrojny July 23, 2023 08:48
@cedricziel cedricziel requested a review from lstrojny July 23, 2023 10:47
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
@cedricziel cedricziel requested a review from brettmc July 23, 2023 11:42
@brettmc
Copy link
Collaborator

brettmc commented Jul 24, 2023

Is everybody happy for me to merge this?

Copy link
Contributor

@lstrojny lstrojny left a comment

Choose a reason for hiding this comment

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

@brettmc sure, just a small cosmetics issue from my side

src/Instrumentation/PDO/src/PDOAttributeTracker.php Outdated Show resolved Hide resolved
@cedricziel cedricziel requested a review from lstrojny July 24, 2023 07:03
@cedricziel
Copy link
Contributor Author

I'm good

@brettmc brettmc merged commit 3a515fa into open-telemetry:main Jul 24, 2023
55 checks passed
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.

3 participants