Skip to content

feat(contrib/jackc/pgx.v5): Add WithErrCheck option#3627

Merged
mtoffl01 merged 16 commits into
DataDog:mainfrom
ryanslade:rs/err-check
Jul 31, 2025
Merged

feat(contrib/jackc/pgx.v5): Add WithErrCheck option#3627
mtoffl01 merged 16 commits into
DataDog:mainfrom
ryanslade:rs/err-check

Conversation

@ryanslade

@ryanslade ryanslade commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

This allows ignoring certain errors so that the spans do not show up as errors later.

What does this PR do?

Motivation

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running golangci-lint run locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@ryanslade
pgx.v5: Add IgnoreError option
baf3c07 
This allows ignoring certain errors so that the spans do not show up as
errors later.
@ryanslade
ryanslade requested review from a team as code owners June 12, 2025 09:52
mtoffl01
mtoffl01 requested changes Jun 25, 2025
View reviewed changes

@mtoffl01 mtoffl01 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @ryanslade , thank you for your contribution!

Can you add a few simple test cases for this feature?

Thanks!

@mtoffl01 mtoffl01 self-assigned this Jun 26, 2025
mtoffl01
mtoffl01 requested changes Jun 30, 2025
View reviewed changes

@mtoffl01 mtoffl01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! I changed the Option logic slightly, as well as the name, to fit our preexisting standard (WithErrCheck -- example). Let me know how this looks to you

Comment thread contrib/jackc/pgx.v5/option.go Outdated
Comment thread contrib/jackc/pgx.v5/option.go Outdated
Comment thread contrib/jackc/pgx.v5/option.go Outdated
Comment thread contrib/jackc/pgx.v5/pgx_tracer.go Outdated
Comment thread contrib/jackc/pgx.v5/pgx_tracer_test.go Outdated
Comment thread contrib/jackc/pgx.v5/pgx_tracer_test.go Outdated
@ryanslade

ryanslade commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

@mtoffl01 Thanks for taking a look, I think I've covered all your comments

@ryanslade ryanslade changed the title pgx.v5: Add IgnoreError option pgx.v5: Add WithErrCheck option Jul 11, 2025
@ryanslade
ryanslade requested a review from mtoffl01 July 11, 2025 13:59
@ryanslade

ryanslade commented Jul 18, 2025

Copy link
Copy Markdown
Contributor Author

@mtoffl01 Would you mind taking a look at this? I think it's good to go 🙇

@genesor

genesor commented Jul 30, 2025

Copy link
Copy Markdown
Member

Hey @ryanslade

Thanks for the PR, it seems good to me. I changed t.Context() to context.Background() as we support Go 1.23 which does not include this func.

I will wait for the CI to run before merging

@genesor genesor changed the title pgx.v5: Add WithErrCheck option feat(contrib/jackc/pgx.v5): Add WithErrCheck option Jul 31, 2025
@genesor genesor added the apm:ecosystem contrib/* related feature requests or bugs label Jul 31, 2025
@ryanslade

ryanslade commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

Hey @ryanslade

Thanks for the PR, it seems good to me. I changed t.Context() to context.Background() as we support Go 1.23 which does not include this func.

I will wait for the CI to run before merging

Amazing, thanks!

@genesor

genesor commented Jul 31, 2025

Copy link
Copy Markdown
Member

Hi @ryanslade

I've made some more changes in the test as it was failing. Turns out that the cancelled context does not trigger span creation as no query is executed (as the context is cancelled it makes sense)

I changed the test here to run two queries that trigger errors and one of them is ignored.

It will merge the PR with the updated test.

@mtoffl01
mtoffl01 merged commit 5d73761 into DataDog:main Jul 31, 2025
109 checks passed
@ryanslade
ryanslade deleted the rs/err-check branch August 14, 2025 11:09
@ryanslade

ryanslade commented Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

@mtoffl01 @eliottness Do you know why this wasn't included in the v2.2.2 release?

@darccio

darccio commented Aug 14, 2025

Copy link
Copy Markdown
Member

@ryanslade This landed on main after v2.2.x release started. We usually do that at the beginning of month. This will be on v2.3.0 that it'll be available around mid September. If you need it, at the beginning of September you'll have the release candidate available.

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

Reviewers

@genesor genesor genesor approved these changes
@mtoffl01 mtoffl01 mtoffl01 approved these changes

Assignees

@mtoffl01 mtoffl01

Labels

apm:ecosystem contrib/* related feature requests or bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ryanslade @genesor @darccio @mtoffl01