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

Metric exporter REUSABLE_DATA memory mode configuration options #6304

Merged

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Mar 18, 2024

The work that has been done to introduce the REUSABLE_DATA memory mode dramatically reduces memory allocations for the metrics SDK. And because the SDK doesn't allow concurrent collections, it should be safe to use in built in exporters like OTLP and prometheus. (The only time it wouldn't be safe to use would be if an exporter held on to MetricData refs after completing the CompletableResultCode returned by export.)

This PR adds experimental API to programmatically set reusable data memory mode:

OtlpHttpMetricExporterBuilder builder = OtlpHttpMetricExporter.builder();
OtlpConfigUtil.setMemoryModeOnOtlpMetricExporterBuilder(builder, MemoryMode.REUSABLE_DATA);

And an experimental system property / env var for setting reusable memory mode:

OTEL_JAVA_EXPERIMENTAL_EXPORTER_MEMORY_MODE=REUSABLE_DATA

This will set memory mode for any exporter that supports setting reusable data memory mode. Currently, that include OtlpHttpMetricExporter, OtlpGrpcMetricExporter, and PrometheusHttpServer.

After we incubate this for a bit, we should make these configuration options part of the permanent public surface area, and probably enable REUSABLE_DATA mode by default for OTLP and prometheusmetrics, since its almost always the right thing to do.

cc @asafm

@jack-berg jack-berg requested a review from a team March 18, 2024 21:08
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 91.09%. Comparing base (1dde255) to head (be86995).
Report is 11 commits behind head on main.

Files Patch % Lines
...lemetry/exporter/otlp/internal/OtlpConfigUtil.java 77.77% 3 Missing and 1 partial ⚠️
...lemetry/exporter/internal/ExporterBuilderUtil.java 77.77% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6304      +/-   ##
============================================
- Coverage     91.11%   91.09%   -0.02%     
- Complexity     5760     5772      +12     
============================================
  Files           627      627              
  Lines         16799    16852      +53     
  Branches       1718     1720       +2     
============================================
+ Hits          15306    15351      +45     
- Misses          998     1006       +8     
  Partials        495      495              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@asafm asafm left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking this. It's on my list, but I haven't gotten around to it yet.

@asafm
Copy link
Contributor

asafm commented Mar 26, 2024

I've added this in the feature implementation plan: #5105 (comment)

@asafm
Copy link
Contributor

asafm commented Mar 31, 2024

@jack-berg Do we want to add support also PrometheusHttpServer in another PR?

@jack-berg
Copy link
Member Author

@jack-berg Do we want to add support also PrometheusHttpServer in another PR?

Yes I think its good to add support to prometheus. Wondering whether we'd want just a single property instead of multiple:

OTEL_JAVA_EXPERIMENTAL_EXPORTER_MEMORY_MODE=REUSABLE_DATA
  • This would apply to all exporters, current and future, which support memory mode configuration
  • As of this PR, only OTLP metric exporter supports this, but we know we want to add support for prometheus exporter, and probably for OTLP span and logger exporters in the future once we optimize the serialization piece.
  • A single property is simple and avoids the proliferation of several experimental env vars.
  • Multiple properties gives you more granularity, but what are the chances you actually want / need it? If you do want more granularity, can implement AutoConfigurationCustomizer.

@asafm
Copy link
Contributor

asafm commented Apr 3, 2024

@jack-berg Do we want to add support also PrometheusHttpServer in another PR?

Yes I think its good to add support to prometheus. Wondering whether we'd want just a single property instead of multiple:

OTEL_JAVA_EXPERIMENTAL_EXPORTER_MEMORY_MODE=REUSABLE_DATA
  • This would apply to all exporters, current and future, which support memory mode configuration
  • As of this PR, only OTLP metric exporter supports this, but we know we want to add support for prometheus exporter, and probably for OTLP span and logger exporters in the future once we optimize the serialization piece.
  • A single property is simple and avoids the proliferation of several experimental env vars.
  • Multiple properties gives you more granularity, but what are the chances you actually want / need it? If you do want more granularity, can implement AutoConfigurationCustomizer.

I favor your single property suggestion

@jack-berg jack-berg changed the title OTLP metric exporter REUSABLE_DATA memory mode configuration options Metric exporter REUSABLE_DATA memory mode configuration options Apr 3, 2024
@jack-berg
Copy link
Member Author

@asafm I've pushed commits to:

  • Generalize the env var. Now OTEL_JAVA_EXPERIMENTAL_EXPORTER_MEMORY_MODE sets memory mode for all exporters which support it
  • Add setter for memory mode PrometheusHttpServerBuilder.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations @jack-berg. Looks like a great start to configure a potentially amazing perf improvement.

@jack-berg jack-berg merged commit 4d8f4f3 into open-telemetry:main Apr 5, 2024
18 checks passed
Copy link
Contributor

@asafm asafm left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, last week I was busy for a lecture I gave in a conference.

this.prometheusRegistry = builder.prometheusRegistry;
this.otelScopeEnabled = builder.otelScopeEnabled;
this.allowedResourceAttributesFilter = builder.allowedResourceAttributesFilter;
this.executor = builder.executor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to copy the memory mode as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

@@ -85,6 +89,11 @@ public AggregationTemporality getAggregationTemporality(InstrumentType instrumen
return prometheusMetricReader.getAggregationTemporality(instrumentType);
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

The only difference I see in PrometheusHttpServer compared with PeriodMetricReader is that in Prometheus, multiple threads can call the metric producer collect(). I think if the mode is turned at REUSABLE_DATA, we should protect and make it only one request at a time.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lock preventing concurrent reads from a particular meter here, but its not enough: A meter could be read from sequentially, but PrometheusHttpServer might still be serializing the MetricData from the first read when the second read begins and overwrites the data. So yes, a lock is needed when PrometheusHttpServer has reusable data enabled. This is interesting because it makes the tradeoff really clear: Do you want reduced memory allocation but no concurrent reads? Or higher memory allocation w/ concurrent reads?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I think we can place that in the javaDoc of the memory mode setter of the builder, once we fix the bug. Do we have place we write documentation of exporters in general ?

| otel.logs.exporter | OTEL_LOGS_EXPORTER | List of exporters to be used for logging, separated by commas. Default is `otlp`. `none` means no autoconfigured exporter. |
| otel.java.experimental.exporter.memory_mode | OTEL_JAVA_EXPERIMENTAL_EXPORTER_MEMORY_MODE | If `reusable_data`, enable reusable memory mode (on exporters which support it) to reduce allocations. Default is `immutable_data`. This option is experimental and subject to change or removal.**[1]** |

**[1]**: NOTE: The exporters which adhere
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you like me to add documentation to the website about memory mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's an issue open somewhere to move all this autoconfigure docs to the website. For the moment, opentelemetry.io just links to this page for anything related to SDK environment variables. I think that's fine for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm asking if we should elaborate more on the memory mode, or the configuration description suffices?

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.

3 participants