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

Add context argument to LogRecordProcessor#onEmit #4889

Merged
merged 2 commits into from Nov 9, 2022

Conversation

jack-berg
Copy link
Member

Alternative to #4888 which adds context as an argument to processor. This allows context to be garbage collected sooner than with #4888 as described here.

Resolves #4147.

@jack-berg jack-berg requested a review from a team as a code owner October 26, 2022 18:54
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 90.83% // Head: 90.87% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (ce7786f) compared to base (21d5597).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4889      +/-   ##
============================================
+ Coverage     90.83%   90.87%   +0.03%     
- Complexity     4857     4860       +3     
============================================
  Files           556      556              
  Lines         14475    14476       +1     
  Branches       1410     1411       +1     
============================================
+ Hits          13149    13155       +6     
+ Misses          908      905       -3     
+ Partials        418      416       -2     
Impacted Files Coverage Δ
.../io/opentelemetry/sdk/logs/LogRecordProcessor.java 85.71% <ø> (ø)
...entelemetry/sdk/logs/SdkLoggerProviderBuilder.java 100.00% <ø> (ø)
...metry/sdk/logs/export/BatchLogRecordProcessor.java 88.97% <ø> (ø)
...etry/sdk/logs/export/SimpleLogRecordProcessor.java 88.57% <ø> (ø)
...pentelemetry/sdk/logs/MultiLogRecordProcessor.java 90.90% <100.00%> (ø)
...opentelemetry/sdk/logs/NoopLogRecordProcessor.java 100.00% <100.00%> (ø)
...io/opentelemetry/sdk/logs/SdkLogRecordBuilder.java 97.43% <100.00%> (+0.06%) ⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️
.../opentelemetry/sdk/logs/SdkReadWriteLogRecord.java 93.54% <0.00%> (+12.90%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx!

@jkwatson
Copy link
Contributor

jkwatson commented Nov 3, 2022

seems fine if the linked issue is really no longer blocked on spec changes

@jack-berg
Copy link
Member Author

jack-berg commented Nov 8, 2022

open-telemetry/opentelemetry-specification#2927 resolves this at the spec level. We discussed having two methods in LogRecordProcessor while the spec gets sorted out. I think the spec will resolve this PR quickly, and so I'm inclined to delay merging this until then.

@jack-berg
Copy link
Member Author

open-telemetry/opentelemetry-specification#2927 has been merged and this is now unblocked.

@jack-berg jack-berg merged commit b60f4e2 into open-telemetry:main Nov 9, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
* Add context argument to LogRecordProcessor#onEmit

* Change argument order
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.

Pass parentContext to LogProcessor.emit(), similar to SpanProcessor.onStart()
4 participants