Repository navigation
Add basic testing for tracing - #6051
Conversation
roji
left a comment
There was a problem hiding this comment.
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.
| { | ||
| { "exception.type", ex.GetType().FullName }, | ||
| { "exception.message", ex.Message }, | ||
| // TODO: only set ex.StackTrace? |
There was a problem hiding this comment.
Hmm, good point... We should check what others (e.g. ASP.NET) are doing here...
There was a problem hiding this comment.
There is a new method Activity.AddException added with .NET 9 which calls Exception.ToString, so we're probably fine...
There was a problem hiding this comment.
I'm actually more interested whether we should set Activity.Status...
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done. Also we now set Activity.Status just in case (remote consumers should already be able to via otel.status_code tag).
There was a problem hiding this comment.
Do we need both Activity.Status and otel.status_code? Maybe we can remove the latter?
| internal static void CommandStop(Activity activity) | ||
| { | ||
| activity.SetTag("otel.status_code", "OK"); | ||
| activity.SetStatus(ActivityStatusCode.Ok); |
There was a problem hiding this comment.
Is this a dup of the line just above? Or are there two different things? Same below for exception.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't know enough about this... What do ASP.NET do, for instance?
There was a problem hiding this comment.
From what I've been able to fine, they just call SetStatus.
There was a problem hiding this comment.
Thanks for looking - I guess we can do the same then?
Closes #4285