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

Allow SpanProcessor#onStart to modify received Span #1569

Closed
mateuszrzeszutek opened this issue Aug 20, 2020 · 9 comments · Fixed by #1574
Closed

Allow SpanProcessor#onStart to modify received Span #1569

mateuszrzeszutek opened this issue Aug 20, 2020 · 9 comments · Fixed by #1574
Assignees
Labels
Feature Request Suggest an idea for this project help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release

Comments

@mateuszrzeszutek
Copy link
Member

Is your feature request related to a problem? Please describe.
There's an inconsistency with the spec: it clearly states that SpanProcessor#OnStart should receive a read/write instance of Span. Current Java SDK implementation receives only a ReadableSpan which makes implementing any Span enrichment/modification in SpanProcessor#onStart() impossible.

Describe the solution you'd like
A proposed solution is to add a second Span parameter to the onStart function - it will provide the "write" capability. I believe that adding a second parameter that represents a write-only span is in accordance with the spec ("...the Span could be one of the parameters passed to such a function, or a getter could be provided...").

Describe alternatives you've considered
Main motivation for this change is open-telemetry/opentelemetry-java-instrumentation#942 . Instead of adding thread info using a SpanProcessor (as was suggested here) I can always just add those attributes directly in the Java agent.

Additional context
Related issues/PRs/comments:

@mateuszrzeszutek mateuszrzeszutek added the Feature Request Suggest an idea for this project label Aug 20, 2020
@jkwatson
Copy link
Contributor

This is relatively recent spec change, and yes, we need to get the implementation updated! Thanks for pointing it out!

@jkwatson jkwatson added release:required-for-ga Required for 1.0 GA release priority:p2 Medium priority issues and bugs. help wanted labels Aug 20, 2020
@jkwatson
Copy link
Contributor

I think we might want another interface for this which joins the readability of the ReadableSpan and the write-capability of the Span itself.

@carlosalberto
Copy link
Contributor

(Or pass both)

@jkwatson
Copy link
Contributor

I think passing both might be extra confusing for users, especially since we'll be passing the same implementation in for both parameters. :)

@carlosalberto
Copy link
Contributor

Fair enough. Let's keep the idea of passing both as plan B ;)

@jkwatson
Copy link
Contributor

I think it's as easy as this:

public interface ReadWriteSpan extends Span, ReadableSpan {
}

Then just have RecordEventsReadableSpan implement that, and change the onStart take the new interface.

I'll PR it. :)

@jkwatson jkwatson self-assigned this Aug 20, 2020
@jkwatson
Copy link
Contributor

it's that easy except for the Disruptor async exporter which needs a new event type.

@mateuszrzeszutek
Copy link
Member Author

I'll PR it. :)

Great, thanks!
Do you have an estimate when you'll finish it?

@jkwatson
Copy link
Contributor

I'll PR it. :)

Great, thanks!
Do you have an estimate when you'll finish it?

I'm working on it this afternoon. 🤞 that I get the disruptor sorted out by then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release
Projects
None yet
3 participants