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

Investigate Potential to bypass LogRecord cloning when emitting LogRecord to LogProcessor(s) #1650

Open
lalitb opened this issue Mar 29, 2024 · 3 comments · May be fixed by #1726
Open

Investigate Potential to bypass LogRecord cloning when emitting LogRecord to LogProcessor(s) #1650

lalitb opened this issue Mar 29, 2024 · 3 comments · May be fixed by #1726

Comments

@lalitb
Copy link
Member

lalitb commented Mar 29, 2024

https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/logs/log_emitter.rs#L210 This clone needs some rethinking. Currently, every processor, even the ones that can stream event directly will have to suffer from this clone cost. I think we should just pass ref to logrecord to processors, and if a processor wants to batch it, then it can do the clone and pay the cost. This should give a significant cost reduction.

Originally posted by @cijothomas in #1649 (comment)

@cijothomas
Copy link
Member

I meant to say - its best if we can do this, irrespective of the number of processors, not just optimize for the 1-processor scenario. Span current does the special case for single processor..But I think it should be possible to always pass ref or mut ref to processors.

@lalitb lalitb changed the title Investigate Potential to Bypass LogRecord Cloning with Single LogProcessor Investigate Potential to bypass LogRecord cloning with emitting LogRecord to LogProcessor(s) Mar 29, 2024
@lalitb
Copy link
Member Author

lalitb commented Mar 29, 2024

updated title accordingly. The problem is, as of now there is no-single owner of the created LogRecord. So, to achieve passing ref/mut-ref, we need to first have an owner of this LogRecord, and then ensure that the record is not dropped while any of the references is being used. Need some investigation.

@lalitb lalitb changed the title Investigate Potential to bypass LogRecord cloning with emitting LogRecord to LogProcessor(s) Investigate Potential to bypass LogRecord cloning when emitting LogRecord to LogProcessor(s) Mar 29, 2024
@lalitb
Copy link
Member Author

lalitb commented Apr 2, 2024

Ok, It was much simpler to add a special case to avoid LogData cloning in case of single processor ( I believe that would be the most common use-case). Have added that change as part of #1636 (as changes are closely tied to that PR). I can see vast improvement in stress test with that:

Main branch:

Number of threads: 8
Throughput: 16,356,200 iterations/sec
Throughput: 15,894,400 iterations/sec
Throughput: 16,018,200 iterations/sec
Throughput: 16,344,800 iterations/sec
PR branch: (with Resource propagation )

PR branch: (with Resource propagation at startup):

Number of threads: 8
Throughput: 19,301,400 iterations/sec
Throughput: 19,789,000 iterations/sec
Throughput: 20,133,600 iterations/sec
Throughput: 19,763,000 iterations/sec

PR branch: (with Resource propagation + avoid LogData clone for single processor)

Number of threads: 8
Throughput: 28,997,800 iterations/sec
Throughput: 29,057,800 iterations/sec
Throughput: 28,755,800 iterations/sec
Throughput: 29,712,000 iterations/sec

@lalitb lalitb linked a pull request May 10, 2024 that will close this issue
4 tasks
@cijothomas cijothomas added this to the Logs beta release milestone May 21, 2024
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 a pull request may close this issue.

2 participants