Enable reusuable_data memory mode by default #6799
Conversation
| .setVersion("1.0") | ||
| .setSchemaUrl("http://url") | ||
| .build()) | ||
| .setInstrumentationScopeInfo(SCOPE_INFO) |
There was a problem hiding this comment.
This change seems random, but is actually oddly needed for a subtle reason. The OTLP exporter tests generate fake test data, serialize and send it to a mock server, where it is deserialized into generated proto classes and evaluated for equality. As you're probably aware, OTLP payloads consist of a series of enclosing envelopes, e.g. for logs ResourceLogs enclose ScopeLogs enclose LogRecords. When an OTLP exporter recieves a List<LogRecordData>, the records need to be grouped by resource and scope to match the OTLP payload structure. We do this using identity hash maps. Notably, the reuseable memory mode serialization iterates over identity the identity hash maps slightly differently than the immutable memory mode variant. This means that when a group of records is exported with scopes which are equal but not identical, the order of those ScopeLogs in the serialized payload is not the same for immutable memory mode and reuseable memory mode.
We can make the tests pass by having the fake log records use identical scopes, which sidesteps the difference in iteration order between immutable memory mode and reuseable memory mode.
There was a problem hiding this comment.
Thanks for the clarification....this is a clever workaround.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6799 +/- ##
============================================
+ Coverage 90.37% 90.48% +0.10%
- Complexity 6577 6590 +13
============================================
Files 731 731
Lines 19714 19721 +7
Branches 1926 1928 +2
============================================
+ Hits 17817 17845 +28
+ Misses 1300 1285 -15
+ Partials 597 591 -6 ☔ View full report in Codecov by Sentry. |
| @@ -82,7 +83,13 @@ public static PrometheusHttpServerBuilder builder() { | |||
| // sequentially. | |||
| if (memoryMode == MemoryMode.REUSABLE_DATA) { | |||
| executor = | |||
| Executors.newSingleThreadExecutor(new DaemonThreadFactory("prometheus-http-server")); | |||
There was a problem hiding this comment.
newSingleThreadExecutor returns a delegating wrapped ThreadPoolExecutor. One of our tests introspects on the resolved executor and ensures that when memory mode is reuseable, a single thread executor is used. The test is not able to make the required assertions based on the delegating wrapped version returned by Executors.newSingleThreadExecutor.
There was a problem hiding this comment.
The way you described it makes it sound like a shortcoming in the test/assertions...which is a bummer that the implementation had to change to accommodate it. It seems fine, but I can't fully reason about the impact of having a non-finalizable executorservice. I'm overthinking it.
| "MemoryMode is REUSEABLE_DATA cannot be used with custom executor, " | ||
| + "since data may be corrupted if reading metrics concurrently"); |
There was a problem hiding this comment.
Is this documented anywhere else?
There was a problem hiding this comment.
we'll need to update the docs and change otel.java.experimental.exporter.memory_mode to otel.java.exporter.memory_mode, so we could potentially add in another footnote when we do that.
...ometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java
Outdated
Show resolved
Hide resolved
| "MemoryMode is REUSEABLE_DATA cannot be used with custom executor, " | ||
| + "since data may be corrupted if reading metrics concurrently"); |
There was a problem hiding this comment.
we'll need to update the docs and change otel.java.experimental.exporter.memory_mode to otel.java.exporter.memory_mode, so we could potentially add in another footnote when we do that.
…y-java into default-memory-mode
This PR follows through on the plan to make reuseable data the default memory mode, as discussed in #6469.
I'm personally aware of it being used in several production systems for a while now without issue. If anyone is aware of any risks associated with this, please speak up! If not, this change in defaults will quietly improve the performance of all the opentelemetry-java users who missed the memory mode optimizations we've been making.