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 file configuration ComponentProvider support for exporters #6493

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

jack-berg
Copy link
Member

Followup to #6457, adding file configuration support for custom SpanExporter, MetricExporter, LogRecordExporter.

As a part of this, adds ComponentProvider implementations for all the exporters maintained in this module.

@jack-berg jack-berg requested a review from a team June 3, 2024 21:05
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 80.67797% with 57 lines in your changes missing coverage. Please review.

Project coverage is 90.50%. Comparing base (3fa57f9) to head (a65a721).

Files Patch % Lines
...lemetry/exporter/otlp/internal/OtlpConfigUtil.java 71.42% 12 Missing and 8 partials ⚠️
...elemetry/sdk/autoconfigure/internal/SpiHelper.java 59.09% 8 Missing and 1 partial ⚠️
...lemetry/exporter/internal/ExporterBuilderUtil.java 22.22% 6 Missing and 1 partial ⚠️
...incubator/fileconfig/LogRecordExporterFactory.java 70.58% 4 Missing and 1 partial ⚠️
...sion/incubator/fileconfig/SpanExporterFactory.java 73.68% 4 Missing and 1 partial ⚠️
...on/incubator/fileconfig/MetricExporterFactory.java 75.00% 3 Missing and 1 partial ⚠️
...ternal/OtlpLogRecordExporterComponentProvider.java 93.54% 1 Missing and 1 partial ⚠️
.../internal/OtlpMetricExporterComponentProvider.java 94.87% 1 Missing and 1 partial ⚠️
...lp/internal/OtlpSpanExporterComponentProvider.java 93.54% 1 Missing and 1 partial ⚠️
...nal/ConsoleLogRecordExporterComponentProvider.java 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6493      +/-   ##
============================================
- Coverage     90.62%   90.50%   -0.13%     
- Complexity     6261     6281      +20     
============================================
  Files           689      696       +7     
  Lines         18704    18845     +141     
  Branches       1844     1843       -1     
============================================
+ Hits          16951    17055     +104     
- Misses         1198     1223      +25     
- Partials        555      567      +12     

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

MetricExporter.class,
exporterKeyValue.getKey(),
exporterKeyValue.getValue());
return FileConfigUtil.addAndReturn(closeables, metricExporter);
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is returning nulls from these Factory implementations a pattern that we like? I looked at the interface definition, and it didn't seem to imply that null return value was acceptable.

interface Factory<ModelT, ResultT> {

  /**
   * Interpret the model and create {@link ResultT} with corresponding configuration.
   *
   * @param model the configuration model
   * @param spiHelper the service loader helper
   * @param closeables mutable list of closeables created
   * @return the {@link ResultT}
   */
  ResultT create(@Nullable ModelT model, SpiHelper spiHelper, List<Closeable> closeables);
}

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 the contract of the parent Factory interface should probably be adjusted.

Currently, the contract is ResultT create(@Nullable ModelT model, SpiHelper spiHelper, List<Closeable> closeables), so it can accept a null model but is always expected to either return (or throw). This is odd. Always returning when the model is null means that every type has to have some no-op or default representation, which is too strict. Throwing when the model is nullable is a weird pattern too. Why allow the model to be nullable if its just going to throw NPE?

So I think we (i.e. me) need to adjust the factory contract to be consistent: Either both the input model and response are nullabe, OR neither is nullable. And it seems best if neither the input or the response is null. This means that callers have to do some additional work to ensure they're not working with a null model, but should be a good thing in net.

If you're on board, I'll open up an issue and follow up. For the purposes of this PR, there is no appropriate MetricExporter to return when the input model is null, so we need to throw an exception or return null. Returning null is preferable because it is handled by the caller and we can generate a more appropriate error message in MetricReaderFactory than we can in MetricExporterFactory:

io.opentelemetry.sdk.metrics.export.MetricExporter metricExporter =
MetricExporterFactory.getInstance().create(exporterModel, spiHelper, closeables);
if (metricExporter == null) {
throw new ConfigurationException("exporter required for periodic reader");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems totally reasonable to me to adjust the interface to reflect what appears to be the reality. The other Option (heh) would be to return Optional<ResultT> from the interface, which might signal the correct semantics? Since this isn't a hot-path interface, I think we should at least consider it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #6612 to refactor away from using nullable in the factory contract. Not sure why I thought that was necessary, but it was clearly a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on using Optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

The need for optional goes away when we embrace avoiding null. Once you eliminate the ability for Factory to accept a @Nullable model, there aren't actually any cases where its useful to return Optional.empty(). When model is always non-null, Factory should always returns a valid component or throws ConfigurationException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! I completely misread what you did over in the other PR. Thanks! Great refactoring of that!

@jkwatson
Copy link
Contributor

Generally seems good to me. A couple of comments provided.

@jack-berg
Copy link
Member Author

@open-telemetry/java-approvers could use a look when you have a chance. Thanks!

@jack-berg jack-berg merged commit ea6e3dd into open-telemetry:main Aug 5, 2024
18 checks passed
breedx-splk pushed a commit to breedx-splk/opentelemetry-java that referenced this pull request Aug 12, 2024
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