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

Add instrumentation for r2dbc-postgresql-1.0.0 #1413

Closed
wants to merge 6 commits into from

Conversation

koenpunt
Copy link
Contributor

@koenpunt koenpunt commented Jul 25, 2023

Before contributing, please read our contributing guidelines and code of conduct.

Overview

Adds updated instrumentation for r2dbc-postgresql-1.0.0, since instrumentation was disabled in #999.

Similar to #1410

One caveat is that 1.0.x introduced support for multiple hosts, but AFAIK there no way to know which host is used at the point where the code is instrumented, so for now I defaulted to the first host in the configuration if there are multiple.

Edit: I mimicked the implementation of the other r2dbc-*sql implementations to get the active socket address, so now the hostname should be correct at all times.

Related Github Issue

Include a link to the related GitHub issue, if applicable

Testing

The agent includes a suite of tests which should be used to
verify your changes don't break existing functionality. These tests will run with
Github Actions when a pull request is made. More details on running the tests locally can be found
here,

Checks

  • Your contributions are backwards compatible with relevant frameworks and APIs.
  • Your code does not contain any breaking changes. Otherwise please describe.
  • Your code does not introduce any new dependencies. Otherwise please describe.

@kford-newrelic kford-newrelic added the estimate Issue needing estimation label Aug 9, 2023
@kford-newrelic kford-newrelic added oct-dec qtr Represents proposed work item for the Oct-Dec quarter 3 Story Point Estimate and removed estimate Issue needing estimation labels Sep 20, 2023
@meiao meiao self-assigned this Oct 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

Merging #1413 (c77d353) into main (7435d77) will increase coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1413      +/-   ##
============================================
+ Coverage     70.60%   70.61%   +0.01%     
+ Complexity     9794     9793       -1     
============================================
  Files           817      817              
  Lines         39489    39489              
  Branches       5995     5995              
============================================
+ Hits          27880    27884       +4     
+ Misses         8902     8898       -4     
  Partials       2707     2707              

see 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@meiao
Copy link
Contributor

meiao commented Oct 23, 2023

While testing this PR, it was found out that the instrumentation for 1.0.0 also worked for 0.9.2.
So the 1.0.0 was changed to be the 0.9.2 instrumentation and that will work up to the latest version.

@koenpunt
Copy link
Contributor Author

Hm, maybe because my initial approach used different apis in those versions, but when later updated the same components were used again..

passesOnly 'org.postgresql:r2dbc-postgresql:[0.9.2,1.0.0.RELEASE)'
excludeRegex(".*(M1|M2|RC).*")
passesOnly 'org.postgresql:r2dbc-postgresql:[0.9.2,)'
exclude 'org.postgresql:r2dbc-postgresql:1.0.0.RC1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Something was odd with this release.
Removed the regex so if there is an incompatibility with any preview of 2.0.0 we will be notified then, instead of when the final version is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a test that ran without instrumentation. Not sure what was the intent of it.
Removing because it took 8 seconds.

@meiao meiao removed their assignment Oct 23, 2023
@meiao
Copy link
Contributor

meiao commented Nov 13, 2023

This PR was merged by #1556

@meiao meiao closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 Story Point Estimate oct-dec qtr Represents proposed work item for the Oct-Dec quarter
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants