-
Notifications
You must be signed in to change notification settings - Fork 848
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 stdout memory mode #6774
add stdout memory mode #6774
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6774 +/- ##
============================================
+ Coverage 90.10% 90.58% +0.48%
- Complexity 6541 6582 +41
============================================
Files 728 731 +3
Lines 19695 19714 +19
Branches 1935 1926 -9
============================================
+ Hits 17746 17858 +112
+ Misses 1349 1259 -90
+ Partials 600 597 -3 ☔ View full report in Codecov by Sentry. |
@jack-berg this is the final PR for stdout - adding the memory mode feature |
|
||
import io.opentelemetry.sdk.common.export.MemoryMode; | ||
|
||
public class OtlpStdoutExporterBuilderUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing internal boilerplate javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did the build not fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a build check for it right now. Do you know if opentelemetry-java-instrumentation
managed to add this automation yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened new issue here: #6798
...main/java/io/opentelemetry/exporter/logging/otlp/internal/OtlpStdoutExporterBuilderUtil.java
Outdated
Show resolved
Hide resolved
...mon/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogReusableDataMarshaler.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/exporter/logging/otlp/internal/logs/OtlpStdoutLogRecordExporter.java
Outdated
Show resolved
Hide resolved
public CompletableResultCode doExport(Marshaler exportRequest, int numItems) { | ||
return jsonWriter.write(exportRequest); | ||
} | ||
}.export(logs); | ||
} else { | ||
for (ResourceLogsMarshaler resourceLogs : ResourceLogsMarshaler.create(logs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to be the case that if wrapperJsonObject = false
, we can't take advantage of reuseable memory mode. Can we include wrapperJsonObject
as an argument to LogReuseableDataMarshaler
and adjust the logic accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not so easy, because
Line 55 in 715211e
public void initialize(Collection<LogRecordData> logDataList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's punt on it for now
...n/java/io/opentelemetry/exporter/logging/otlp/internal/metrics/OtlpStdoutMetricExporter.java
Show resolved
Hide resolved
d5021a9
to
cfabe23
Compare
@jack-berg can you trigger the failed link check? otherwise, everything should be fine |
...mon/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogReusableDataMarshaler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good besides the missing internal boilerplate javadoc. Thanks!
No description provided.