Improve design of internal/stanza log converter #3125
Conversation
|
Testbed results comparison: this PR: |
pmalek-sumo
force-pushed
the
improved-logs-translation-workerpool
branch
2 times, most recently
from
01332b7 to
f01f1ac
Compare
pmalek-sumo
force-pushed
the
improved-logs-translation-workerpool
branch
from
f01f1ac to
4414374
Compare
|
@djaglowski Let me know whether there's any sense in me rebasing this and resolving conflicts so that we'd like to keep looking at it or reject it straight away. |
|
@pmalek-sumo This design is certainly easier to reason about. It's unfortunate that we lose much of the performance gains from the previous PR, but I think we are still coming in slightly ahead, and perhaps this design gives us a path to further improvement. One thing I am a little concerned about is testing. Do you think we're sufficiently exercising various configurations? One thought I have on the converter, is that it's quite difficult to use for testing purposes. It would be nice if there were a way to "statelessly" convert |
We can easily add those it depends what you have in mind. This package is transitively tested by other packages which use it so this somewhat makes me more confident that it works hence I didn't spend too much time on testing it. I'm willing to put more effort if there's will to accept it on this (not only) condition :).
I believe we can add a thin wrapper |
|
@pmalek-sumo Thanks, I think it makes sense to move forward on this approach. It's looking promising. |
pmalek-sumo
force-pushed
the
improved-logs-translation-workerpool
branch
from
4414374 to
655f40b
Compare
|
@djaglowski Added Not sure what sort of tests we'd like to add. As I've mentioned: this being transitively as a direct dependency of e.g. filelogreceiver or syslogreceiver is rather sufficient. One small point in this subject I'd like to raise is that if we observe testbed tests to fail we can then address it (of course if need be). |
There was a problem hiding this comment.
This looks great to me. Thanks @pmalek-sumo, for all the iterations on this one.
|
Irrelevant |
|
Please rebase |
pmalek-sumo
force-pushed
the
improved-logs-translation-workerpool
branch
from
b23197f to
bfa9291
Compare
@bogdandrutu Done. I can see that testbed test |
Just noting that there is an issue for this already: #3095 For now, I'm rerunning tests. |
Benchmarks Before: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitLogs BenchmarkSplitLogs-16 16072 76529 ns/op 114408 B/op 669 allocs/op PASS ``` Benchmarks After: ``` goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitLogs BenchmarkSplitLogs-16 21795 56491 ns/op 88952 B/op 556 allocs/op PASS ``` Benchmarks Reference Clone: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkCloneLogs BenchmarkCloneLogs-16 22305 52075 ns/op 85976 B/op 503 allocs/op PASS ``` Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Description: Improve the internal design of internal/stanza log converter.
workerLoop,batchLoop,flushLoopTesting: existing unit tests pass + added a converter benchmark