Skip to content

Conversation

bantonsson
Copy link
Contributor

Motivation

Since OtelData is no longer public, there needs to be a way to access the OpenTelemetry context from other Layers and ensure that the context is created.

Solution

Introduce a SpanRefExt that given a SpanRef and the Dispatch for the layers will get or create the OpenTelemetry context.

Note

Both the test and the example has a workaround for tokio-rs/tracing#3379 which makes the code unnecessarily complicated.

@mladedav
Copy link
Collaborator

I'll have a closer look later, but on first look, I have some reservations. But maybe they'll be solvable in this design.

The first thing I noticed when skimming this is that SpanRefExt::context should definitely take &mut self because it internally takes an exclusive lock on its extensions which would definitely lead to people deadlocking themselves through that.

I don't think this makes sense to rush this now so I'll leave it out of today's release and go with the simpler version that does not start the context automatically.

@bantonsson
Copy link
Contributor Author

Thanks for noticing @mladedav. That is definitely a problem. No need to rush it.

I don't think that a &mut self will protect against the locking issue. I think that the ExtensionMut needs to be passed into the function to make it explicit.

@mladedav
Copy link
Collaborator

You're right, for some reason I though when you have ExtensionMut it mutably borrows the SpanRef but it's just normal borrow (and on top of that you can just get another SpanRef by callign ctx.span(&span.id)).

Passing the extensions explicitly is also probably better since the user will most likely already have the lock or will want to get it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants