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

[Logs SDK] Send resource once to processor and exporter, and not for every event. #1636

Merged
merged 34 commits into from
Apr 30, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Mar 21, 2024

Fixes Part of #1124 (for logs)
Design discussion issue (if applicable) #

Changes

  1. Send resource once to processor at the startup:
    a. Add LogExporter::set_resource(&resource) method.
    b. Add LogProcessor::set_resource(&resource) method, which invokes set_resource on all the exporters.
    c. The LogExporter would store the resource in original or serialized format, and merge it with each event during export.
    d. The LoggerProvider invokes set_resource on all the processors during its initialization.
    e. Remove resource from the LogData structure, which is exported to the exporters through LogExporter::emit(logdata). This ensures that resource is not sent to the exporter with each event.
    2. Avoid cloning LogRocord during emit if single processor is configured. Will create separate PR for this

call sequence -
LoggerProvider::build() -> LogProcessor::set_resource() -> LogExporter::set_resource()

Currently, these changes apply exclusively to Logs. Once there is consensus and these changes are merged, will extend them to traces and metrics. Additionally, changes made in #1526 can be undone after implementing this across all signals.

Benchmark:

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 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 at startup + avoid LogRecord copy for single processor) will create separate PR for this

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

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 55.02959% with 76 lines in your changes are missing coverage. Please review.

Project coverage is 69.2%. Comparing base (f203b03) to head (0634469).
Report is 17 commits behind head on main.

❗ Current head 0634469 differs from pull request most recent head 3576450. Consider uploading reports for the commit 3576450 to get more accurate results

Files Patch % Lines
opentelemetry-otlp/src/exporter/http/logs.rs 0.0% 13 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/logs.rs 0.0% 13 Missing ⚠️
opentelemetry-sdk/src/logs/log_emitter.rs 57.6% 11 Missing ⚠️
opentelemetry-proto/src/transform/logs.rs 0.0% 10 Missing ⚠️
opentelemetry-stdout/src/logs/transform.rs 0.0% 9 Missing ⚠️
opentelemetry-proto/src/transform/common.rs 0.0% 6 Missing ⚠️
opentelemetry-stdout/src/logs/exporter.rs 0.0% 6 Missing ⚠️
opentelemetry-otlp/src/logs.rs 0.0% 3 Missing ⚠️
opentelemetry-sdk/src/logs/log_processor.rs 95.2% 3 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs 0.0% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1636     +/-   ##
=======================================
- Coverage   69.3%   69.2%   -0.1%     
=======================================
  Files        136     136             
  Lines      19637   19561     -76     
=======================================
- Hits       13610   13544     -66     
+ Misses      6027    6017     -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lalitb lalitb changed the title [WIP] Send resource once to processor and exporter, and not for every event. [Logs SDK] Send resource once to processor and exporter, and not for every event. Mar 28, 2024
@lalitb lalitb marked this pull request as ready for review March 28, 2024 07:33
@lalitb lalitb requested a review from a team March 28, 2024 07:33
@cijothomas
Copy link
Member

@lalitb any noticeable change in performance, out of curiosity!

@cijothomas
Copy link
Member

Looks good overall! Thank you.
Will wait couple days to get more eyes on this approach!

@lalitb
Copy link
Member Author

lalitb commented Mar 28, 2024

@lalitb any noticeable change in performance, out of curiosity!

Yes, there is a visible improvement with stress-test in the hot path as we don't clone and add resources in the LogData during emit. There should be improvement in memory usage too, but we don't have a direct way of measuring it.

with PR

$ cargo run --bin logs --all-features --release
   Compiling opentelemetry_sdk v0.22.1 (/home/labhas/otel/rust/lalit/set-resource/opentelemetry-rust/opentelemetry-sdk)
   Compiling stress v0.1.0 (/home/labhas/otel/rust/lalit/set-resource/opentelemetry-rust/stress)
    Finished `release` profile [optimized] target(s) in 4.21s
     Running `target/release/logs`
Number of threads: 8
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

main branch

$ cargo run --bin logs --all-features --release
   Compiling opentelemetry_sdk v0.22.1 (/home/labhas/otel/rust/lalit/set-resource/opentelemetry-rust/opentelemetry-sdk)
   Compiling opentelemetry-appender-tracing v0.3.0 (/home/labhas/otel/rust/lalit/set-resource/opentelemetry-rust/opentelemetry-appender-tracing)
   Compiling stress v0.1.0 (/home/labhas/otel/rust/lalit/set-resource/opentelemetry-rust/stress)
    Finished `release` profile [optimized] target(s) in 4.22s
     Running `target/release/logs`
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

@lalitb
Copy link
Member Author

lalitb commented Apr 2, 2024

Have update this PR to not clone LogRecord within Logger:emit() if only single processor is configured - I believe this would be the common use-case. And can see significant improvement in the stress test. Have updated the latest stresss test in the PR description.

@lalitb
Copy link
Member Author

lalitb commented Apr 26, 2024

Seems merge conflicts with #1585, fixing them now.

@lalitb
Copy link
Member Author

lalitb commented Apr 26, 2024

Seems merge conflicts with #1585, fixing them now.

should be good now.

stress/src/logs.rs Outdated Show resolved Hide resolved
@lalitb
Copy link
Member Author

lalitb commented Apr 26, 2024

Please don't merge yet. OTLP http log export seems to be broken after merging changes with #1585. Looking into it

@lalitb
Copy link
Member Author

lalitb commented Apr 26, 2024

Please don't merge yet. OTLP http log export seems to be broken after merging changes with #1585. Looking into it

Should be good now. Need to add integration tests for OTLP Log exporter to detect these issues. Will create a PR for this later.

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