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

Move environment resource to :sdk:all #5544

Closed

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Jun 15, 2023

Another attempt to find the right home for environment resource logic. Related to #5464.

Summary of options:

  • (option A) opentelemetry-java-instrumentation#8604: Move to instrumentation.
    • Pros: All resource providers are in same place. No awkward move of ConfigProperties.
    • Cons: not aligned with spec which implies environment resource logic should live in core.
  • (option B) Copy ConfigProperties, ConfigurationException to opentelemetry-sdk-co… #5500: Move ConfigProperties, environment resource to :sdk:common.
    • Pros: Environment resource lives alongside core resource logic.
    • Cons: Moving ConfigProperties is awkward and will be hard to explain to users.
  • (option c) Move autoconfigure getConfig to internal, remove getResource #5467: Keep environment resource in autoconfigure. Can adjust to add dedicated EnvironmentResource accessor class and corresponding ResourceProvider.
    • Pros: Simple. No awkward move of ConfigProperties.
    • Cons: autoconfigure doesn't contain any other SPI implementations. Feels weird to take autoconfigure dependency if only accessing EnvironmentResource programatically. Does living in autoconfigure meet the spirit of the specification?
  • (option d) This PR: Move environment resource to :sdk:all.
    • Pros: Environment resource logic stays in java core repo, and doesn't require dependency on autoconfigure. No need for awkward move of ConfigProperties
    • Cons: Feels awkward to have this logic in :sdk:all. No other similar code lives there.

This seems like a pretty exhaustive set of options. Should try to pick our preferred solution so we can unblock stabilization of autoconfigure (this is really the only thorny issue).

@jack-berg jack-berg requested a review from a team June 15, 2023 19:56
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.01 🎉

Comparison is base (cf3b0ef) 91.38% compared to head (54559b1) 91.39%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5544      +/-   ##
============================================
+ Coverage     91.38%   91.39%   +0.01%     
- Complexity     4960     4963       +3     
============================================
  Files           549      551       +2     
  Lines         14566    14564       -2     
  Branches       1356     1356              
============================================
  Hits          13311    13311              
+ Misses          868      867       -1     
+ Partials        387      386       -1     
Impacted Files Coverage Δ
...lp/http/logs/OtlpHttpLogRecordExporterBuilder.java 90.00% <ø> (ø)
...lp/http/metrics/OtlpHttpMetricExporterBuilder.java 95.23% <ø> (ø)
...r/otlp/http/trace/OtlpHttpSpanExporterBuilder.java 90.00% <ø> (ø)
...lemetry/exporter/otlp/internal/OtlpConfigUtil.java 89.84% <ø> (ø)
...r/otlp/internal/OtlpLogRecordExporterProvider.java 100.00% <ø> (ø)
...rter/otlp/internal/OtlpMetricExporterProvider.java 100.00% <ø> (ø)
...porter/otlp/internal/OtlpSpanExporterProvider.java 100.00% <ø> (ø)
...elemetry/exporter/otlp/internal/OtlpUserAgent.java 100.00% <ø> (ø)
...er/otlp/logs/OtlpGrpcLogRecordExporterBuilder.java 91.66% <ø> (ø)
...er/otlp/metrics/OtlpGrpcMetricExporterBuilder.java 100.00% <ø> (ø)
... and 5 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jack-berg
Copy link
Member Author

After some careful consideration, I'm in favor of option c:

  • Add public API surface area for accessing the resource derived from environment variables and system properties.
  • Establish a precedent that upon request, we can expose public APIs in autoconfigure for accessing other core SDK components configured via environment variables / system properties. For example, upon request we could add an API for programmatically obtaining a BatchSpanProcessor with properties set from the environment variables.
  • Also consider adding public API surface area for accessing exporters configured from the environment (currently internal APIs like OtlpLogRecordExporterProvider)

Reasoning:

  • To meet the spirit of the spec, we should really have the code that derives the resource from environment live in the core repository. That eliminates option a.
  • Ideally, we'd extend the pattern of distributing the code which interprets config and configures components to live in the artifact of the component it configures. I.e. just as OtlpLogRecordExporterProvider, we'd ideally have the code that configures Resource / BatchSpanProcessor / SpanLimits / etc from the environment live in the core SDK repo. But this is hard because of circular dependencies. option d (this PR) gets around the circular dependency issues by putting the code in :sdk:all, but that doesn't feel quite right. I think its reasonable to say that code for configuring core SDK components from the environment live in autoconfigure.
  • Users who want to consume the EnvironmentResource code programatically can do so by taking a dependency on autoconfigure. Taking this dependency is no sweat, even if they don't fully use autoconfigure, because autoconfigure is small and contains no problematic dependencies.

If others agree, I'll close this PR and others, leaving just #5467 open. I'll adjust #5467 to add a new public API for accessing the resource from the environment.

public int order() {
// A high order ensures that the environment resource takes precedent over the resources from
// other ResourceProviders
return 10;
Copy link
Member

Choose a reason for hiding this comment

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

Consider making this a larger number:

Suggested change
return 10;
return 10_000;

or even 1_000_000. We have a couple of resource providers in instrumentation/contrib that use values 100/200/1000, and they should definitely run before this one. Also a large order number doesn't really change anything, and it does provide some extra flexibility when it comes to customization.

@@ -51,6 +52,6 @@ void getAttributesWithFuzzing() {
"getAttributesWithRandomValues",
new NoGuidance(10000, System.out),
System.out);
assertThat(result.wasSuccessful()).isTrue();
Assertions.assertThat(result.wasSuccessful()).isTrue();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed?

Suggested change
Assertions.assertThat(result.wasSuccessful()).isTrue();
assertThat(result.wasSuccessful()).isTrue();

@jack-berg
Copy link
Member Author

Closing in favor of #5554.

@jack-berg jack-berg closed this Jun 20, 2023
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