-
Notifications
You must be signed in to change notification settings - Fork 849
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
Initialize configuration factory #5687
Initialize configuration factory #5687
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #5687 +/- ##
============================================
+ Coverage 91.24% 91.26% +0.02%
Complexity 5045 5045
============================================
Files 558 558
Lines 14924 14924
Branches 1397 1397
============================================
+ Hits 13618 13621 +3
+ Misses 899 897 -2
+ Partials 407 406 -1 ☔ View full report in Codecov by Sentry. |
448c7d3
to
854b537
Compare
cleanup.addCloseable(sdk); | ||
cleanup.addCloseables(closeables); | ||
|
||
assertThat(sdk.toString()).isEqualTo(expectedSdk.toString()); |
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.
These are pretty cool tests. We create a configuration model, parse / interpret it, and compare the toString()
result to an expected version we build up manually. Its a bit strange to use toString
as a stand in for equality, but I think it makes sense in this case because these aren't data carrier objects.
// opentelemetry-configuration | ||
if (example.getName().equals("anchors.yaml")) { | ||
continue; | ||
} |
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.
@trask an unexpected side affect of switching from snakeyaml to snakeyaml-engine is that the merge symbol isn't supported anymore, which reduces the ability to use anchors.
With merge, you can reference an anchor and extend it with additional elements:
my-anchor: &my-anchor
foo: bar
my-object:
<<: *my-anchor
baz: qux
With anchor support, should result in:
my-object:
foo: bar
baz: qux
But instead results in:
my-object:
<<:
foo: bar
baz: qux
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 like a great start to me.
.../main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/LogRecordExporterFactory.java
Show resolved
Hide resolved
@open-telemetry/java-approvers take a look when you can. Want to get this merged so I can proceed with the trace / metric implementations. |
.../opentelemetry/sdk/extension/incubator/fileconfig/OpenTelemetryConfigurationFactoryTest.java
Outdated
Show resolved
Hide resolved
…y-java into initialize-configuration-factory
This builds on #5399 by laying the groundwork for interpreting the parsed
OpenTelemetryConfiguration
model.Some notes:
ConfigProperties
. When this matures, there will be an opportunity to unify the code. For example, we may be able to convert the environment variable / system property scheme to an equivalentOpenTelemetryConfiguration
model.Factory
interface, with contractResultT create( @Nullable ModelT model, ServiceLoaderHelper serviceLoaderHelper, List<Closeable> closeables)
.ServiceLoaderHelper
to load any SPIs if needed.List<Closeables>
with any closeable components it builds, so allow for cleanup of partials if something goes wrong.OpenTelemetryConfigurationFactory
, which resolves a completeOpenTelemetrySdk
.ConfigurationFactory
, which has an APIpublic static OpenTelemetrySdk parseAndInterpret(InputStream inputStream)
, responsible for parsing an input stream of YAML configuration to a OpenTelemetryConfiguration model, and interpreting that model to createOpenTelemetrySdk
.