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

Batch Processor: Log Support #1723

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Batch Processor: Log Support #1723

merged 1 commit into from
Sep 2, 2020

Conversation

keitwb
Copy link
Contributor

@keitwb keitwb commented Sep 2, 2020

This mimics the same logic as metrics for logs in the batchprocessor.

@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #1723 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1723   +/-   ##
=======================================
  Coverage   92.37%   92.38%           
=======================================
  Files         258      258           
  Lines       18192    18239   +47     
=======================================
+ Hits        16805    16850   +45     
- Misses        975      976    +1     
- Partials      412      413    +1     
Impacted Files Coverage Δ
consumer/pdata/log.go 100.00% <100.00%> (ø)
internal/data/testdata/log.go 100.00% <100.00%> (ø)
processor/batchprocessor/batch_processor.go 98.49% <100.00%> (+0.34%) ⬆️
processor/batchprocessor/factory.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 87.03% <0.00%> (-1.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b5b45...f346762. Read the comment docs.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also enable batcher in the e2e log tests where relevant (if the similar trace tests use the batcher)?

processor/batchprocessor/batch_processor.go Outdated Show resolved Hide resolved
}

func (bm *batchLogs) size() int {
return bm.logData.ResourceLogs().Len()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is size() supposed to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the metrics batcher, the size in bytes of the internal data representation of logs. I added a helper method to pdata to derive it without the batch processor having to convert to internal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have batching by size in bytes? I thought we only support batching by the number of items. Can you point me to the code which does byte-based batching? I don't see anything mentioned in the config of the batcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just used for telemetry at the detailed level to record how many bytes the batches are.

@keitwb
Copy link
Contributor Author

keitwb commented Sep 2, 2020

Can we also enable batcher in the e2e log tests where relevant (if the similar trace tests use the batcher)?

I'll look into this. The trace correctness test uses batch processor.

@tigrannajaryan tigrannajaryan self-assigned this Sep 2, 2020
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tigrannajaryan
Copy link
Member

@keitwb Please resolve the conflict.

This mimics the same logic as metrics for logs in the batchprocessor.
@keitwb
Copy link
Contributor Author

keitwb commented Sep 2, 2020

Can we also enable batcher in the e2e log tests where relevant (if the similar trace tests use the batcher)?

I'll look into this. The trace correctness test uses batch processor.

Yeah it is probably best to add e2e tests for logging (beyond the existing perf test) with another PR/issue as it looks quite involved.

@tigrannajaryan tigrannajaryan merged commit 889948e into open-telemetry:master Sep 2, 2020
@keitwb keitwb deleted the logs-batch-proc branch September 8, 2020 17:17
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants