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

Allow logs to be mutated by LogProcessor #4643

Merged
merged 4 commits into from
Aug 11, 2022

Conversation

jack-berg
Copy link
Member

Implements spec PR#2681.
Resolves #4550.

Summary:

  • Rename LogBuilder to LogRecordBuilder to better reflect the data model and the naming we can expect to see in the log appender / event API.
  • Change LogProcessor#emit to onEmit to better match trace SDK.
  • Introduce ReadWriteLogRecord interface, which is passed to LogProcessor#onEmit instead of LogData. This is roughly aligned with tracing. However, in tracing ReadWriteSpan extends Span and ReadableSpan. This separation of Span for updating and ReadableSpan for reading isn't needed in the log SDK because there isn't a LogProcessor onStart and onEnd which have different read / write semantics.
  • ReadWriteLogRecord has a toLogData() method, which is called by the simple and batch log processor to convert the data to an immutable representation before calling LogExporter#export(Collection<LogData>).
  • Add single attribute setters (setAttribute(AttributeKey<T>, T)), rename multi attribute setter to setAllAttributes(Attributes) to mirror tracing

@jack-berg jack-berg requested a review from a user July 27, 2022 22:54
@jack-berg jack-berg requested a review from Oberon00 as a code owner July 27, 2022 22:54
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #4643 (caff25a) into main (77be2e0) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##               main    #4643      +/-   ##
============================================
- Coverage     90.08%   90.06%   -0.02%     
- Complexity     5074     5085      +11     
============================================
  Files           584      586       +2     
  Lines         15667    15707      +40     
  Branches       1504     1507       +3     
============================================
+ Hits          14114    14147      +33     
- Misses         1100     1103       +3     
- Partials        453      457       +4     
Impacted Files Coverage Δ
...n/java/io/opentelemetry/sdk/logs/LogProcessor.java 85.71% <ø> (ø)
...lemetry/sdk/logs/SdkLogEmitterProviderBuilder.java 100.00% <ø> (ø)
...va/io/opentelemetry/sdk/logs/LogRecordBuilder.java 60.00% <60.00%> (ø)
.../opentelemetry/sdk/logs/SdkReadWriteLogRecord.java 80.00% <80.00%> (ø)
...entelemetry/sdk/logs/export/BatchLogProcessor.java 88.72% <85.71%> (ø)
...io/opentelemetry/sdk/logs/SdkLogRecordBuilder.java 97.50% <92.85%> (ø)
...a/io/opentelemetry/sdk/logs/MultiLogProcessor.java 90.90% <100.00%> (ø)
...va/io/opentelemetry/sdk/logs/NoopLogProcessor.java 100.00% <100.00%> (ø)
.../java/io/opentelemetry/sdk/logs/SdkLogEmitter.java 100.00% <100.00%> (ø)
...ntelemetry/sdk/logs/export/SimpleLogProcessor.java 88.57% <100.00%> (ø)
... and 1 more

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

@jack-berg
Copy link
Member Author

Hey @jkwatson - wondering what your thoughts are about trying to get this in before the 1.17.0 release. It technically depends on this spec PR, but I think its extremely likely that the spec goes in this direction and that its current state of not allowing mutable logs is accidental. Additionally, both the spec and the sdk are experimental so I think its ok for us to experiment (within reason!).

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

I'm fine with this. It'll be interesting to see how it ends up differing from the final spec, but we'll update as appropriate before stabilization.

*
* <p>Note: the behavior of null values is undefined, and hence strongly discouraged.
*/
<T> ReadWriteLogRecord setAttribute(AttributeKey<T> key, T value);
Copy link
Member Author

Choose a reason for hiding this comment

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

@trask for now the only thing you can mutate is log attributes. I plan on adding getters / setters on here in the future, but wondering if this is sufficient for your short term requirements.

Copy link
Member

Choose a reason for hiding this comment

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

yes, this is exactly what I need for now 👍

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.

How to modify log data in the log pipeline?
3 participants