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

span-reporter span id generation is very slow. #18 #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rohitprg
Copy link

This is the PR for issue #18
Span-reporter should have an option to use the tracing implementation's span id instead of generating its own.
Span-reporter method SpanBuilderR.findSpanId calls context.baggageItems() twice.

… span id instead of generating its own + Avoid call to context.baggageItems() twice in SpanBuilderR.findSpanId
@rohitprg
Copy link
Author

rohitprg commented Oct 25, 2021

Span-reporter uses UUID.randomUUID() to generate the span id, which internally uses SecureRandom and is very slow.

Whereas tracer server like Jaeger uses Java6CompatibleThreadLocalRandom.current().nextLong() which is 10 times faster with no contention(one thread) and around 50 to 100 times faster with contention (multilple threads) compared to UUID.randomUUID().

Instead of creating its own span id, the span-reporter lib can get the span id from the tracing implementation.

Can someone please review the MR? Any feedback would be highly appreciated.

@rohitprg
Copy link
Author

rohitprg commented Feb 23, 2022

Reason behind second commit (Override toString() method)

Calling toString() on the Tracer when using span-reporter does not return useful data.
For example, calling GlobalTracer.get().toString() returns:

"GlobalTracer{io.opentracing.contrib.reporter.TracerR@5eb97ced}"

However, if you do the same thing when directly using the Jaeger tracing you get:

"GlobalTracer{JaegerTracer(version=Java-1.6.0, serviceName=boss, reporter=RemoteReporter(sender=HttpSender(), closeEnqueueTimeout=1000), sampler=ProbabilisticSampler(tags={sampler.type=probabilistic, sampler.param=1.0}), tags={hostname=ffff-dev, jaeger.version=Java-1.6.0, java.user.name=bmhhhh, java.version=11.0.13, boss.signature=0.1.0-SNAPSHOT/f4bc2fc3/fffff/2021-11-17T18:40:34Z, ip=**************}, zipkinSharedRpcSpan=false, expandExceptionLogs=false, useTraceId128Bit=false)}"

io.opentracing.contrib.reporter.TracerR needs to implement toString and return a value that includes the data returned by the wrapped tracer.

@rohitprg
Copy link
Author

It has been a while since I opened the merge request. Can someone please review and provide the access to merge?

@rohitprg
Copy link
Author

rohitprg commented Mar 4, 2022

@davidB Can you please review the pull request?

Copy link
Collaborator

@davidB davidB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution,
I need to go back (refresh) into the API of Opentracing (like spanContext.toSpanId();) and into the code to complete my review.
FYI, I'll not be able to publish a new release (I don't have the permissions to publish)

Comment on lines +79 to +81
else {
referenceId = findSpanId(spanContext);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding the impl of findSpanId, referenceId is always "" in this case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, I need to go back (refresh) into the API of Opentracing (like spanContext.toSpanId();) and into the code to complete my review. FYI, I'll not be able to publish a new release (I don't have the permissions to publish)

@davidB - Thanks for your reply. Does that mean there wont be any releases/updates on this library going forward?

@sriramraghavan-oracle
Copy link

Hello @davidB and @bhs
Can you guys help in closing this fix and help with a new release?
Thanks

@bhs
Copy link
Contributor

bhs commented May 10, 2022

It has been an eternity since I worked on this code and I can't even build it anymore, my apologies! My own efforts in the overall space are completely focused on opentelemetry these days (opentracing has been deprecated).

Not the answer you're hoping for, I'm sure, but I wanted to respond at least. :-/

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.

4 participants