Add basic testing for tracing by vonzshik · Pull Request #6051 · npgsql/npgsql

vonzshik · 2025-03-16T16:59:32Z

roji

Thanks, this looks great! If you're really in the mood for observability testing, I remember there's also a package specifically for helping test metrics too.

roji · 2025-03-17T13:26:49Z

src/Npgsql/NpgsqlActivitySource.cs

        {
            { "exception.type", ex.GetType().FullName },
            { "exception.message", ex.Message },
+            // TODO: only set ex.StackTrace?


Hmm, good point... We should check what others (e.g. ASP.NET) are doing here...

There is a new method Activity.AddException added with .NET 9 which calls Exception.ToString, so we're probably fine...

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L595

I'm actually more interested whether we should set Activity.Status...

There is a new method Activity.AddException added with .NET 9

Would be nice to use that even if it doesn't do much else (maybe it will at some point?)

Would be nice to use that even if it doesn't do much else (maybe it will at some point?)

Well, whenever we actually upgrade from .NET 8...

We can multi-target to .NET 9 at any time for Npgsql 10, though if it's just for that may indeed not be worth it... Can at least add a comment though.

Done. Also we now set Activity.Status just in case (remote consumers should already be able to via otel.status_code tag).

Do we need both Activity.Status and otel.status_code? Maybe we can remove the latter?

roji · 2025-03-17T16:49:57Z

src/Npgsql/NpgsqlActivitySource.cs

    internal static void CommandStop(Activity activity)
    {
        activity.SetTag("otel.status_code", "OK");
+        activity.SetStatus(ActivityStatusCode.Ok);


Is this a dup of the line just above? Or are there two different things? Same below for exception.

Not exactly. Activity.Status (and Activity.Description) do not add any tags, they're essentially just fields in Activity and it's up to exporters to populate tags depending on Activity.Status. So in case there's an exporter which doesn't react on Activity.Status, then these tags will never be added if we remove them from there.
I'm more-or-less OK with removing explicit tags.

I don't know enough about this... What do ASP.NET do, for instance?

From what I've been able to fine, they just call SetStatus.

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/61164064e25a111ad141d1f0cf2c8d329e16683a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L263-L266

Thanks for looking - I guess we can do the same then?

Updated [Npgsql](https://github.com/npgsql/npgsql) from 9.0.3 to 10.0.2. <details> <summary>Release notes</summary> _Sourced from [Npgsql's releases](https://github.com/npgsql/npgsql/releases)._ ## 10.0.2 v10.0.2 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/135?closed=1) **Full Changelog**: npgsql/npgsql@v10.0.1...v10.0.2 ## 10.0.1 v10.0.1 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/134?closed=1) **Full Changelog**: npgsql/npgsql@v10.0.0...v10.0.1 ## 10.0.0 See the [release notes](https://www.npgsql.org/doc/release-notes/10.0.html). The full list of changes is available [here](https://github.com/npgsql/npgsql/milestone/122?closed=1). ## What's Changed * STJ 9.0 alternative approach by @NinoFloris in npgsql/npgsql#5941 * Remove support for net6.0 by @roji in npgsql/npgsql#5947 * Some leftover cleanup for removing net6.0 by @roji in npgsql/npgsql#5949 * Map date/time to DateOnly/TimeOnly by default by @roji in npgsql/npgsql#5948 * Make the cidr<->IPNetwork mapping the default by @roji in npgsql/npgsql#5950 * Fix connecting with VerifyCA and VerifyFull by @vonzshik in npgsql/npgsql#5944 * Remove stopwatch allocations by @vonzshik in npgsql/npgsql#5977 * Bump actions/setup-dotnet from 4.1.0 to 4.2.0 by @dependabot[bot] in npgsql/npgsql#5983 * Use exception convenience methods by @bbowyersmyth in npgsql/npgsql#5982 * Bump actions/setup-dotnet from 4.2.0 to 4.3.0 by @dependabot[bot] in npgsql/npgsql#6007 * Add support for postgresql type names with dots by @dvas-hash in npgsql/npgsql#5971 * Send close_notify TLS alert on connection shutdown by @vonzshik in npgsql/npgsql#5995 * Remove DisplayClass struct creation in PgReader by @bbowyersmyth in npgsql/npgsql#6014 * Always dispose RemoteCertificate on SslStream by @vonzshik in npgsql/npgsql#6022 * Remove LongRunningConnection field from NpgsqlConnector by @vonzshik in npgsql/npgsql#6024 * Tighten SCRAM-SHA-256 SASL check by @vonzshik in npgsql/npgsql#6023 * Add SHA3 hash algorithms for SASL authentication by @vonzshik in npgsql/npgsql#6028 * Remove dotnet SDK version from CI (use global.json) by @roji in npgsql/npgsql#6037 * Add support for specifying allowed auth methods by @vonzshik in npgsql/npgsql#6036 * Migrate to SLNX by @roji in npgsql/npgsql#6053 * Switch to Ubuntu 24.04 in CI by @roji in npgsql/npgsql#6054 * Bump actions/setup-dotnet from 4.3.0 to 4.3.1 by @dependabot[bot] in npgsql/npgsql#6059 * Add basic testing for tracing by @vonzshik in npgsql/npgsql#6051 * parameter-collection Clone() should set correct collection instance by @mgravell in npgsql/npgsql#6066 * Fix brew on mac CI by @NinoFloris in npgsql/npgsql#6071 * Fix adding to hash lookup while renaming an unnamed parameter by @vonzshik in npgsql/npgsql#6073 * Update LICENSE date (2024 -> 2025) by @kurnakovv in npgsql/npgsql#6082 * Add tracing for physical connection open by @vonzshik in npgsql/npgsql#6091 * Start testing on .NET 9 by @vonzshik in npgsql/npgsql#5945 * Turn on <IsAotCompatible> by @roji in npgsql/npgsql#6097 * Reenable public API analyzer by @roji in npgsql/npgsql#6101 * Update Npgsql to .NET 9 by @vonzshik in npgsql/npgsql#6099 * Ignore system CA store if root certificate is provided by @vonzshik in npgsql/npgsql#6102 * Fix reading columns asynchronously via JsonNet plugin by @vonzshik in npgsql/npgsql#6109 * Fixes #6107 missed should buffer in biginteger numeric converter by @NinoFloris in npgsql/npgsql#6117 * Fix logging parameters with batches by @vonzshik in npgsql/npgsql#6079 * Implement GSSAPI session encryption by @vonzshik in npgsql/npgsql#6131 * feat: add support for PGAPPNAME to set application name by @michael-todorovic in npgsql/npgsql#6139 * Fix returning null from KerberosUsernameProvider.GetUsername with concurrent calls by @vonzshik in npgsql/npgsql#6137 * Add NpgsqlTsVector.Empty by @roji in npgsql/npgsql#6145 * Add assert to NpgsqlCommand.Transaction if it's completed by @vonzshik in npgsql/npgsql#6151 * Compare normalized type names by @0MG-DEN in npgsql/npgsql#6011 * Do CI testing for PG18 (beta) by @roji in npgsql/npgsql#6155 * Fix infinite consume on error with connection break by @vonzshik in npgsql/npgsql#6161 * Bump actions/checkout from 4 to 5 by @dependabot[bot] in npgsql/npgsql#6174 ... (truncated) ## 10.0.0-rc.1 ## 9.0.5 v9.0.5 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/131?closed=1) **Full Changelog**: npgsql/npgsql@v9.0.4...v9.0.5 ## 9.0.4 v9.0.4 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/127?closed=1) **Full Changelog**: npgsql/npgsql@v9.0.3...v9.0.4 Commits viewable in [compare view](npgsql/npgsql@v9.0.3...v10.0.2). </details> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=Npgsql&package-manager=nuget&previous-version=9.0.3&new-version=10.0.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

vonzshik added 3 commits March 16, 2025 19:59

Add basic testing for tracing

1627ef1

Add a few more tests

c8af841

Also test batches

be733c4

vonzshik marked this pull request as ready for review March 17, 2025 12:12

vonzshik requested a review from roji as a code owner March 17, 2025 12:12

roji approved these changes Mar 17, 2025

View reviewed changes

Set status on Activity

ddb3b66

roji reviewed Mar 17, 2025

View reviewed changes

vonzshik added 3 commits March 19, 2025 13:21

Do not set otel tags explicitly

0ad520f

Fix tests

ceea7dd

A few more test fixes

e01b396

vonzshik merged commit 22a0aa9 into main Mar 20, 2025
14 checks passed

vonzshik deleted the 4285-tracing-basic-testing branch March 20, 2025 09:55

This was referenced Jan 5, 2026

Bump the npgsql group with 3 updates VMelnalksnis/Gnomeshade#1809

Closed

Bump Npgsql from 8.0.8 to 10.0.1 Aaronontheweb/akka-reminders#47

Merged

Bump the nuget-all group with 22 updates amieldelatorre/records#142

Closed

This was referenced Jan 13, 2026

Bump the npgsql group with 3 updates VMelnalksnis/Gnomeshade#1819

Closed

Bump the nuget-all group with 21 updates amieldelatorre/records#143

Closed

Bump the all group with 5 updates open-policy-agent/opa-sdk-demos#386

Open

This was referenced Feb 2, 2026

Bump Npgsql from 9.0.4 to 10.0.1 SharpMUSH/SharpMUSH#413

Merged

deps(nuget): Bump the persistence group with 3 updates FreeSideNomad/ngcp#8

Merged

This was referenced Feb 9, 2026

chore(deps): Bump the all-dependencies group with 3 updates dimi7rof/io.backend#9

Closed

chore(deps): Bump the all-dependencies group with 3 updates dimi7rof/io.backend#11

Closed

This was referenced Feb 20, 2026

Bump Npgsql from 9.0.3 to 10.0.1 mauroservienti/welcome-to-the-state-machine-demos#464

Merged

chore: Bump Npgsql from 9.0.4 to 10.0.1 andy-c-jones/DarkPeak.Functional#33

Merged

dependabot bot mentioned this pull request Mar 9, 2026

Bump Npgsql from 9.0.3 to 10.0.1 RoySalisbury/HVO.Enterprise.Telemetry#131

Open

This was referenced Mar 16, 2026

nuget: Bump Npgsql from 9.0.3 to 10.0.2 aaron-lee-hebert/net-worth-tracker#41

Open

chore(deps): Bump the all-dependencies group with 3 updates dimi7rof/io.backend#13

Open

Bump Npgsql from 9.0.3 to 10.0.2 awslabs/aurora-dsql-connectors#231

Merged

dependabot bot mentioned this pull request Mar 16, 2026

chore(deps): Bump Npgsql from 9.0.4 to 10.0.2 Questo/Ratatosk#33

Merged

Conversation

vonzshik commented Mar 16, 2025

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants