Add InMemoryLogExporter #3757
Add InMemoryLogExporter #3757
Conversation
jack-berg
requested review from
anuraaga,
bogdandrutu,
carlosalberto,
jkwatson and
jsuereth
as code owners
|
Probably not actually a continuation of the one you reference, since that seems to be completely unrelated. ;) |
| */ | ||
| @Override | ||
| public CompletableResultCode export(Collection<LogData> logs) { | ||
| if (isStopped) { |
There was a problem hiding this comment.
do we need this check for some reason? Would it hurt to just not worry about whether shutdown has been called or not?
There was a problem hiding this comment.
I don't have a strong preference, but this is consistent with InMemorySpanExporter and InMemoryMetricExporter.
There was a problem hiding this comment.
Hrm. I wonder why that was done and why it matters.
There was a problem hiding this comment.
Perhaps its easier to test the flush / shutdown methods from the SdkMeterProvider and SdkTracerProvider with a well behaved in memory exporter. 🤷
There was a problem hiding this comment.
I turned off the functionality in the Span version and it didn't seem to break anything. Agreed: 🤷🏽
| /** A {@link LogExporter} implementation that can be used to test OpenTelemetry integration. */ | ||
| public final class InMemoryLogExporter implements LogExporter { | ||
|
|
||
| // using LinkedBlockingQueue to avoid manual locks for thread-safe operations |
There was a problem hiding this comment.
Don't think we need this comment
| public final class InMemoryLogExporter implements LogExporter { | ||
|
|
||
| // using LinkedBlockingQueue to avoid manual locks for thread-safe operations | ||
| private final Queue<LogData> finishedLogItems = new LinkedBlockingQueue<>(); |
There was a problem hiding this comment.
Nit, I'd usually use ConcurrentLinkedQueue since we don't want any blocking behavior (in practice, the usage means it doesn't matter too much though)
There was a problem hiding this comment.
I lifted this from InMemoryMetricExporter. Looking closer, InMemorySpanExporter is inconsistent as it uses ArrayList<>().
I'd like to make all three consistent, and change them all to use Collections.synchronizedList(new ArrayList<>()), which should thread safe, non-blocking (except for synchronization), and plenty fast for the use case. Thoughts?
There was a problem hiding this comment.
no strong opinion. These classes are for testing, so any thread-safe, insertion-order preserving collection should do. And, yes, consistency would be good to have.
Codecov Report
@@ Coverage Diff @@
## main #3757 +/- ##
============================================
+ Coverage 89.08% 89.13% +0.04%
- Complexity 3917 3951 +34
============================================
Files 470 472 +2
Lines 12344 12442 +98
Branches 1208 1225 +17
============================================
+ Hits 10997 11090 +93
+ Misses 945 938 -7
- Partials 402 414 +12
Continue to review full report at Codecov.
|
…y-java into in-memory-log-exporter
| * @return a {@code List} of the finished {@code Log}s. | ||
| */ | ||
| public List<LogData> getFinishedLogItems() { | ||
| synchronized (finishedLogItems) { |
There was a problem hiding this comment.
Who'd have expected such nits on this PR :P
I don't know if it's an implementation detail or not that new ArrayList(list) should be threadsafe because it delegates to toArray, not using an iterator. If others agree that it should be safe, then we can remove the synchronized.
But I do know that it's unfortunate to have to take a lock anyways despite using a sychronized structure... I would suggest sticking to the ConcurrentLinkedQueue, or unsynchronized with lock in each method, vs this hybrid.
There was a problem hiding this comment.
new ArrayList(list) does delegate to toArray(), but there shouldn't be any performance impact of this synchronized keyword because toArray() has to obtain a lock on the list instance already. So synchronized just obtains the lock earlier, and then its a reentrant lock so it doesn't matter.
So in this case, synchronized is just extra defense against the new ArrayList() implementation.
With that said, looking at the internals, ConcurrentLinkedQueue's toArray() implementation is free of synchronization. Since we don't care about size() of ConcurrentLinkedQueue not being constant time, I think its a good option 👍
There was a problem hiding this comment.
Thanks! Just want to clarify performance was never a concern for these testing classes :) Just didn't want inconsistency in the patterns for accessing the variable (some methods used synchronized, others don't). Now that's fixed :)
| * @return a {@code List} of the finished {@code Log}s. | ||
| */ | ||
| public List<LogData> getFinishedLogItems() { | ||
| synchronized (finishedLogItems) { |
There was a problem hiding this comment.
Who'd have expected such nits on this PR :P
I don't know if it's an implementation detail or not that new ArrayList(list) should be threadsafe because it delegates to toArray, not using an iterator.
But I do know that it's unfortunate to have to take a lock anyways despite using a sychronized structure... I would suggest sticking to the ConcurrentLinkedQueue, or unsynchronized with lock in each method, vs this hybrid.
Continuation of #3749. A
InMemoryLogExportermakes it easier to write tests for log appenders.