Conversation
…spans/internal/SamplingProfiler.java
|
@jackshirazi does the CI failure on this PR look like same or different issue than you were trying to fix here? https://scans.gradle.com/s/gfqf5jlzidn6q |
|
it's related. I left shutdown() instead of shutdownNow() which meant the previous test session can on occasion not finish before the next is started. I've added an interrupt for that too with the latest commit |
|
another failure in the latest SamplingProfilerTest: https://scans.gradle.com/s/2dbaqboab5fr6 |
|
lol, this is not getting better |
|
@SylvainJuge and/or @JonasKunz I need a careful review, this has turned into significantly more extensive changes than I expected. there's a bunch of different things
|
| } | ||
|
|
||
| /** For testing only. */ | ||
| public InferredSpansProcessorBuilder tempDir(@Nullable File tempDir) { |
There was a problem hiding this comment.
[minor] not related to this PR, but we could probably replace usage of File with Path to prevent having to call toFile.
There was a problem hiding this comment.
leaving this for another time
| if (profilingTask != null) { | ||
| profilingTask.cancel(true); | ||
| } |
There was a problem hiding this comment.
[minor] I would suggest to also wrap the access to profilingTask and potentially modifying its state with the profilerLock, at least for consistency with the other call to .cancel(true) above. There is however no side effect on potentially calling this twice as the return value of cancel(...) is ignored.
There was a problem hiding this comment.
leaving this for another time
| if (fileSize == 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is the case of empty file covered by any existing test ? I haven't seen any change related to this in the tests.
There was a problem hiding this comment.
good point, test added now
| class SamplingProfilerTest { | ||
|
|
||
| static { | ||
| ProfilingActivationListener.ensureInitialized(); |
There was a problem hiding this comment.
what is the benefit of explicitly calling this in the tests and not in production code ? is there any benefit of doing this and if yes maybe a comment could be welcome.
There was a problem hiding this comment.
the ProfilingActivationListener has a static initializer which has to be executed before any Context access. This happens in production, but is not guaranteed in test because we start things out of order to setup tests
| if (e.getMessage() != null && e.getMessage().contains("already started")) { | ||
| logger.fine("Profiler already started. Stopping and restarting."); | ||
| try { | ||
| profiler.stop(); | ||
| } catch (RuntimeException ignore) { | ||
| logger.log(Level.FINE, "Ignored error on stopping profiler", ignore); | ||
| } | ||
| startMessage = profiler.execute(startCommand); | ||
| } else { | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Is is a known failure mode of async profiler here ? If so, then maybe having a dedicated method to wrap this "when exception is thrown stop, start and try again" logic could make it slightly more readeable.
There was a problem hiding this comment.
this is testing fragility under concurrent test runs, I'd be surprised to see it get hit in production. Especially as dynamic updates to the test durations will be few and far in between in reality
|
|
||
| /** For testing only. */ | ||
| InferredSpansProcessorBuilder activationEventsFile(@Nullable File activationEventsFile) { | ||
| public InferredSpansProcessorBuilder activationEventsFile(@Nullable File activationEventsFile) { |
There was a problem hiding this comment.
are you sure these need to be added to the public api?
There was a problem hiding this comment.
good catch, thanks. They don't
4 participants
closes #2458