Skip to content
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

Log receivers - Improve translation to pdata, avoid emitting entire pdata.Logs for each log record. #2330

Closed
djaglowski opened this issue Feb 11, 2021 · 9 comments

Comments

@djaglowski
Copy link
Member

djaglowski commented Feb 11, 2021

Currently this translation is 1:1. Need to define and implement a more efficient translation.

@pmalek-sumo
Copy link
Contributor

I'd be willing to work on this but I'll need some guidance on the desired direction of this.

My understanding is that there is a func Convert(obsLog *entry.Entry) pdata.Logs
which does the actual conversion.

If we'd like to somehow make this not 1:1 translation then ... we'd need to aggregate entry.Entrys and then using a particular interval send them further for processing? Or is there a different vision for this that I'm not seeing yet?

@djaglowski
Copy link
Member Author

djaglowski commented Mar 4, 2021

I think it makes sense to accumulate entry.Entry's into []entry.Entry and pass this to Convert([]entry.Entry), which will then batch and convert to a single pdata.Logs. There is an implementation of this here, which may be a good starting point.

  • Accumulation of entry.Entry's into []entry.Entry could take place here.

  • []entry.Entry should probably be released based when either a max number of entries is reached, or a timeout is reached.
    Perhaps in a subsequent PR, the max number and max time can be made configurable as part of BaseConfig.

  • We can group entries together when entry.Entry.Resource fields are a perfect match.

  • Are there any other ways in which entries should be batched? Maybe someone more familiar with pdata.Logs can weigh in?

@pmalek-sumo
Copy link
Contributor

I've prepared something that might address this issue in https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/issue-2330-improved-logs-translation

Compare across forks: main...SumoLogic:issue-2330-improved-logs-translation

@djaglowski If this is more or less the direction we'd like to this in I can add some tests, refactor it a little bit and submit a PR so that we can take it from there.

@djaglowski
Copy link
Member Author

@pmalek-sumo This is more or less what I was expecting. Looks like a great start.

@pmalek-sumo
Copy link
Contributor

I've created #2694 as a proposal for this issue.

Current implementation allows configuration via 2 knobs:

  • flush_trigger_count - amount of entries aggregated so far in current batch. If this is exceeded then a flush of (all aggregated) log entries will be scheduled
  • flush_interval - an interval at which flushes will be scheduled.

Waiting for your feedback.

tigrannajaryan pushed a commit that referenced this issue Apr 9, 2021
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:** #2330

**Testing:** unit tests added
@tigrannajaryan
Copy link
Member

@pmalek-sumo @djaglowski do we expect more improvements or we consider done with this?

@pmalek-sumo
Copy link
Contributor

I'd like to propose a variant of #2949 for consideration as design improvement.

The only concern I have about this not being accepted though is that it increases the cpu consumption by a few percent.
If there's will to take a look at it ( a rebased, cleaned up version of #2949) I'll prepare it then

@djaglowski
Copy link
Member Author

@pmalek-sumo If you are ok with it, I would like to consider this issue done.

We can open a new issue to track further proposed changes.

cc @tigrannajaryan

@tigrannajaryan
Copy link
Member

Sounds good. I am closing this issue. We can continue looking at #2949

pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this issue Apr 28, 2021
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
ljmsc referenced this issue in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
Import path incorrect for signal, added import "os/signal".

Also the current online instructions are broken, ctx is not plumped through, but it is here in the forked markdown, so probably needs to be pushed out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants