FISH-5636 Remote EJB Tracing Improvements #5410
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Andrew Pielage <[email protected]>
…nd only get tracer once in StandardWrapper Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]> FISH-5636 Correct try-catch Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
| // Does this NEED to be synchronised? Tracers don't store state, and Scopes are ThreadLocal | ||
|
|
||
| // Double-checked locking - potentially naughty | ||
| Tracer tracer = tracers.get(applicationName); |
There was a problem hiding this comment.
How about ConcurrentHashMap.computeIfAbsent and put the following code into the compute function?
| return requestTracingService.isRequestTracingEnabled(); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| public RequestTracingService getRequestTracingService() { |
There was a problem hiding this comment.
How about returning an Optional?
There was a problem hiding this comment.
I don't think returning an Optional really adds much here.
The method is intended as a poke to HK2 if required rather than any kind of true getter (public scope currently present is wrong - should be private).
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
Signed-off-by: Andrew Pielage <[email protected]>
jGauravGupta
approved these changes
Sep 17, 2021
|
Jenkins test please |
JamesHillyard
pushed a commit
to JamesHillyard/Payara
that referenced
this pull request
Oct 28, 2021
FISH-5636 Remote EJB Tracing Improvements
JamesHillyard
added a commit
to JamesHillyard/Payara
that referenced
this pull request
Dec 23, 2021
…unity" This reverts commit 12c988f.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
An attempt to improve the throughput of the OpenTracing service, particularly with regards to remote EJBs.
The main change is the un-synchronising of the
getTracermethod - creation of tracers is still synchronised but is now split off into another method and utilises double-checked locking to prevent conflicts. While double-checked locking can be naughty, in this case it's being done on a map itself, so the issue of partially constructed references not returning null Shouldn't™ occur.My results from performance testing were largely inconclusive since the realm of variance is pretty large - this is working at the micro and millisecond timeframe, any slight deviation outside of my control (e.g. OS runs a background process) will affect the result. In any case, I shall list the reasoning of how each change should help below.
getTracersplit into two methods:getTracerandcreateTracergetTraceris now unsynchronised, meaning there should be less contention waiting for locks in normal operation since onlycreateTraceris synchronised (tracers only get created once)StandardWrapperno longer does two calls forgetTracerback-to-back (less of an impact now thatgetTraceris no longer synced but still saves performing the get operation on a ConcurrentHashMap twice)RequestTracingServicemoved fromSpanBuilderconstructor toOpenTracingService(and then passed toTraceras a constructor parameter). The intent here is to make it so an HK2 lookup doesn't need to be performed each time a span is created, and is instead done once.This PR also includes some cleanup of deprecated or ugly code:
logwas missing and so I added it inGlobalTracer.register(Tracer)is deprecated, so has been replaced withGlobalTracer.registerIfAbsent(Callable)GlobalTracer.registerIfAbsent(Callable)Eventsfrom constructor ofRequestTraceStoreFactory- not used.DAYvariable removed fromRequestTracingService- not used.ScopeManagermoved fromOpenTracingServicetoTracer- it isn't used anywhere else, and ourTracerimpl doesn't use any other kind ofScopeManagerImportant Info
Dependant PRs
MicroProfile OpenTracing TCK Runner update: payara/MicroProfile-TCK-Runners#153
Blockers
None
Testing
New tests
None - with the level of sensitivity being worked on here (milliseconds) performance tests are not suitable for day-to-day testing.
Testing Performed
Ran my own test using JMeter numerous times against various points on the branch and compared against master - results largely inconclusive. Added my own logging around every call to
getTracerto try and get a sense of the response time of synchronised vs. un-synchronised (inconclusive).Ran a test created by @fturizo and inspected using JProfiler numerous times against various points on the branch and compared against master to see if any discernible difference could be gained (inconclusive).
Ran MicroProfile OpenTracing TCK to check everything still works.
Test suites executed
MicroProfile OpenTracing
Testing Environment
Windows 10, JDK8.
Documentation
N/A
Notes for Reviewers
Please keep performance in mind when reviewing, pointing out stuff I've missed or things I may have misunderstood.