Fixes baggage propagation when an exception is thrown from middleware #2487
Conversation
jamescrosswell
requested review from
bitsandfoxes and
SeanFeldman
as code owners
| Hub fullHub => fullHub.StartTransaction(context, customSamplingContext, dynamicSamplingContext), | ||
| HubAdapter adapter => adapter.StartTransaction(context, customSamplingContext, dynamicSamplingContext), | ||
| _ => hub.StartTransaction(context, customSamplingContext) |
There was a problem hiding this comment.
I don't get what's happening here. Could you explain?
Do we still need that in the first place? It looks to me like we can drop the whole extension method. What other types besides Hub and HubAdapter are we expecting?
There was a problem hiding this comment.
I wonder why does it matter the type at all? Should the overload with DS be in the public interface? Or that information be part of customSamplingContext (even if internal)?
There was a problem hiding this comment.
I don't get what's happening here. Could you explain?
There's a bit of an explanation here. Essentially the DSC was being dropped because of the codepath that extension forced things down.
What other types besides Hub and HubAdapter are we expecting?
DisabledHub is the other one...
There was a problem hiding this comment.
I wonder why does it matter the type at all? Should the overload with DS be in the public interface? Or that information be part of
customSamplingContext(even ifinternal)?
It's pretty unlikely any of our SDK users would want to use this overload... which is why I steered away from putting it on the IHub interface.
We could put it on IHubEx, which is an internal interface only implemented by Hub and HubAdapter (not by DisabledHub).
Potentially we could have DisabledHub implement IHubEx as well... instead of having a bunch of switch expressions all over the place. The HubAdapter seems to pass quite a lot of work off to the SentrySdk, which in turn just pushes it through to the CurrentHub. HubExtensions and SentrySDK both employ switch expression in multiple locations to determine whether to call a real internal method or the public disabled method (that pipes to /dev/null or returns default). That could probably be achieved more simply by having DisabledHub do the same directly if it implemented that internal IHubEx interface.
I can take a look at trying to refactor that if you like, but probably as part of a separate issue to this, as it's not really related to fixing this particular issue.
As an aside, I was a bit surprised to encounter an instance of HubAdapter when running an ASP.NET Core application. The XML docs for that class indicate it was created to help with testing... it seems to offload all of its work to the SentrySdk, which uses a single static Hub instance. Outside of testing, in what scenarios would we expect people to be using the SentrySdk.CurrentHub vs scoped/transient instances of a Hub?
Created Chore: Refactor HubExtensions and SentrySdk #2490 to follow up. |
jamescrosswell
deleted the
fix/baggage-propagation-middleware-exception-2441
branch
Fixes #2441