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 TestLogData, remove public LogDataImpl and LogDataBuilder from log SDK #4635

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

jack-berg
Copy link
Member

I'm working on refactoring some of the Log SDK to allow modifying log records in LogProcessor for #4550 (related spec PR 2681). I have a commit that includes all the necessary changes to make this happen, but in an effort to make the review more manageable, have split out one of the changes into this PR.

Currently there is a a single LogData interface which is accepted by LogProcessor and LogExporter. The goal is to instead pass a ReadWriteLogRecord interface (akin to ReadWriteSpan) to LogProcessor, while reserving LogData for LogExporter#export. This PR removes the public LogDataImpl and LogDataBuilder classes from the Log SDK, replacing them with a TestLogData autovalue class in :sdk:logs-testing for testing purposes. This makes LogData much more in line with SpanData.

@jack-berg jack-berg requested a review from a user July 26, 2022 16:40
@jack-berg jack-berg requested a review from Oberon00 as a code owner July 26, 2022 16:40
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #4635 (83d76cd) into main (2ef73c8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4635      +/-   ##
============================================
- Coverage     90.05%   90.04%   -0.01%     
+ Complexity     5066     5056      -10     
============================================
  Files           581      581              
  Lines         15618    15608      -10     
  Branches       1497     1498       +1     
============================================
- Hits          14065    14055      -10     
  Misses         1098     1098              
  Partials        455      455              
Impacted Files Coverage Δ
...io/opentelemetry/sdk/testing/logs/TestLogData.java 100.00% <100.00%> (ø)
.../java/io/opentelemetry/sdk/logs/SdkLogBuilder.java 100.00% <100.00%> (ø)
...ain/java/io/opentelemetry/sdk/logs/SdkLogData.java 100.00% <100.00%> (ø)
.../java/io/opentelemetry/sdk/logs/SdkLogEmitter.java 100.00% <100.00%> (ø)
...metry/sdk/metrics/export/PeriodicMetricReader.java 87.14% <0.00%> (-2.86%) ⬇️
...y/exporter/internal/marshal/CodedOutputStream.java 71.00% <0.00%> (+1.18%) ⬆️

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 2ef73c8...83d76cd. Read the comment docs.

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.

seems reasonable to me. Just one question about some of the test code

@jack-berg jack-berg merged commit a808649 into open-telemetry:main Jul 27, 2022
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.

2 participants