Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export spanFromContext #452

Closed
wants to merge 1 commit into from
Closed

Export spanFromContext #452

wants to merge 1 commit into from

Conversation

GTB3NW
Copy link

@GTB3NW GTB3NW commented Jun 22, 2022

Matches the logic from TransactionFromContext and returns nil.
This is idiomatic and does what it says on the tin, if the context
does not have a span contained within it, I would not expect anything
to be returned. An error seems... extra.

Matches the logic from `TransactionFromContext` and returns nil.
This is idiomatic and does what it says on the tin, if the context
does not have a span contained within it, I would not expect anything
to be returned. An error seems... extra.
@rhcarvalho
Copy link
Contributor

cc: @rhcarvalho - Not sure if you work for sentry anymore; If you do please could you weigh in on this? The function was originally created by you an annotated with some ideas for future plans.

Hi @GTB3NW! What are you going to use SpanFromContext for? You can't do much with spans without a parent transaction, so I believe TransactionFromContext is still the right choice for most cases.

@GTB3NW
Copy link
Author

GTB3NW commented Jun 23, 2022

cc: @rhcarvalho - Not sure if you work for sentry anymore; If you do please could you weigh in on this? The function was originally created by you an annotated with some ideas for future plans.

Hi @GTB3NW! What are you going to use SpanFromContext for? You can't do much with spans without a parent transaction, so I believe TransactionFromContext is still the right choice for most cases.

Hey, I'm working on a proof of concept at the moment to present this. At the moment the trace and span are sent over the network to remote systems, that means a new span can be created which links to the previous span. The internals of the sentry sdk then see a parentspan id (without a parent span pointer) and a new transaction can be started. This becomes a problem if your edge is not network based. If I want to create a new transaction linked to an existing one and as a child of a span it's impossible without exposing the trace id & most importantly the span id, this allows me to do it in one fell swoop. As you noted in the todo there's other use cases such as modifying instrumented spans from middleware etc. Once I've got the proof of concept sorted I'll send it over if it helps demonstrate the problem.
PS: I have another PR in which it extends the idea above of spawning a new child transaction in-process.

@rhcarvalho
Copy link
Contributor

Would be interesting to see more details on the PoC :-)

If I want to create a new transaction linked to an existing one and as a child of a span it's impossible without exposing the trace id & most importantly the span id, this allows me to do it in one fell swoop.

Trace propagation is done with a sentry-trace string / HTTP header, and includes both trace and span IDs, so exposing those is expected. If you want to hide that info within a context.Context, you can do so using your own context key and then fetch it and start a new transaction using the recently added ContinueFromTrace (#434).

As you noted in the todo there's other use cases such as modifying instrumented spans from middleware etc.

That's right. The "transaction" concept is however so central to Sentry's tracing such that without an associated transaction all span data is garbage collected. In this model, user code can always poke at the spans starting off a transaction.
So some form of "middleware" calling TransactionFromContext to mutate the transaction/spans would be essentially the same as setting up an event processor to go over transactions before they are sent to Sentry.

@GTB3NW
Copy link
Author

GTB3NW commented Jun 23, 2022

Would be interesting to see more details on the PoC :-)

Just packaging it up now

Trace propagation is done with a sentry-trace string / HTTP header, and includes both trace and span IDs, so exposing those is expected. If you want to hide that info within a context.Context, you can do so using your own context key and then fetch it and start a new transaction using the recently added ContinueFromTrace (#434).

Yeah I saw that function but without any way to the data required for the function it's useless. I wouldn't mind adding a function to extract that data from a context as you say. I think my next PR may be a bit more controversial than that change :P

That's right. The "transaction" concept is however so central to Sentry's tracing such that without an associated transaction all span data is garbage collected. In this model, user code can always poke at the spans starting off a transaction.
So some form of "middleware" calling TransactionFromContext to mutate the transaction/spans would be essentially the same as setting up an event processor to go over transactions before they are sent to Sentry.

I'll be honest I'm less bothered about that use case, I personally wouldn't be a user of it, however I am working on some middleware which does need the span/traceid, so less about mutation and more about information :)

@GTB3NW
Copy link
Author

GTB3NW commented Jun 23, 2022

Okay, I've created the POC over - https://github.com/GTB3NW/sentry-poc
It uses the fork for the other PR where I've added the ability to strip the span off an existing context chain.

@GTB3NW
Copy link
Author

GTB3NW commented Jun 23, 2022

I've got an idea for an alternative setup to achieve what I'm trying to do. I'm not as familiar with the internals as you are but in theory I could add a new function to sentry-go which returns a span option which mutates the required fields (which aren't exposed). I don't think we should tack it on to the existing TransactionName function since it would be intrusive to existing users of the function, instead opting for a new ForceTransactionName function which does the same thing but additionally allows us to pass this gate - https://github.com/getsentry/sentry-go/blob/master/tracing.go#L296

This pull request was closed.
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