-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Improved and batched logs translation #2892
Improved and batched logs translation #2892
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2892 +/- ##
==========================================
- Coverage 91.55% 91.53% -0.02%
==========================================
Files 465 465
Lines 22848 22962 +114
==========================================
+ Hits 20918 21019 +101
- Misses 1437 1446 +9
- Partials 493 497 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Does this resolve all comments raised in #2694 ? Please resolve merge conflicts. |
5eb053b
to
d869864
Compare
@tigrannajaryan There are just 2 that might be worth discussing/agreeing on:
Another thing that I'm looking into and I believe might be worth raising a separate issue to track is to check if implementing this PR with a configurable number of workers which would be responsible for converting the entries in their separate goroutines and then they would pipe them through a channel where the aggregation would happen (with the already implemented flushing mechanism). |
I've run testbed tests on this PR once more to post them here for reference:
|
I have also prepared a draft PR #2949 with my approach at adding worker pool to this PR which I believe is a better design due to separation of concerns (smaller, independent units of work, no lock contention, communication via channels). Unfortunately with current state of that PR there a slightly higher resource consumption. I'm attaching the report from testbed tests:
|
Worker pool aside for the moment, I do see an improvement in performance on these changes alone, and the code looks good. I think we just need a few more tests to meet code coverage standards. Relevant results of
The same on this branch:
|
@djaglowski do you mind if I assign this to you to review? |
@tigrannajaryan No problem! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmalek-sumo would it be possible for you to add an ASCII diagram that shows how Converter works, what goroutines exist, how they interact using channels? It would help tremendously to understand the logic.
I can try to sketch something up 👍 |
d869864
to
b69f891
Compare
I've added a diagram in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Nice diagram!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmalek-sumo
Introduce an aggregation layer to internal/stanza that translates [entry.Entry](https://github.com/open-telemetry/opentelemetry-log-collection/blob/83ae56123ba0bd4cd284c3a20ed7450a606af513/entry/entry.go#L43-L51) into pdata.Logs aggregating logs coming from the same Resource into one entry. **Link to tracking Issue:** open-telemetry#2330 **Testing:** unit tests added
Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.38.0 to 1.39.0. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.38.0...v1.39.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Introduce an aggregation layer to internal/stanza that translates [entry.Entry](https://github.com/open-telemetry/opentelemetry-log-collection/blob/83ae56123ba0bd4cd284c3a20ed7450a606af513/entry/entry.go#L43-L51) into pdata.Logs aggregating logs coming from the same Resource into one entry. **Link to tracking Issue:** open-telemetry#2330 **Testing:** unit tests added
Description: Introduce an aggregation layer to internal/stanza that translates entry.Entry into pdata.Logs aggregating logs coming from the same Resource into one entry.
Link to tracking Issue: #2330
Testing: unit tests added