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

Initialize file configuration #5399

Merged
merged 12 commits into from
Aug 3, 2023

Conversation

jack-berg
Copy link
Member

Lay the groundwork for future file configuration work. Some details:

  • The file configuration schema is specified using JSON Schema here
  • This PR defines a gradle task which downloads and unzips the opentelemetry-configuration repo
  • Uses the popular jsonSchema2Pojo to generate POJO from the schema, annotated with Jackson annotations that allow jackson databind to read to it
  • Adds a simple ConfigurationReader class which reads a YAML InputStream and returns the corresponding OpenTelemetryConfiguration representation of it.

We're defining the configuration schema iteratively in small PRs, but eventually a YAML config file might look something like this.

Our goal should be to provide functionality to parse the YAML, and interpret it to build an OpenTelemetrySdk instance. Eventually, we should plug this into the autoconfigure module so to provide an alternative to the environment variable based scheme.

@jack-berg jack-berg requested a review from a team April 24, 2023 15:46
@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7ae32c9) 91.31% compared to head (d9b0350) 91.31%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5399   +/-   ##
=========================================
  Coverage     91.31%   91.31%           
- Complexity     4978     4980    +2     
=========================================
  Files           554      554           
  Lines         14731    14735    +4     
  Branches       1376     1376           
=========================================
+ Hits          13451    13456    +5     
+ Misses          884      883    -1     
  Partials        396      396           

see 1 file with indirect coverage changes

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

implementation(project(":semconv"))
compileOnly("com.sun.net.httpserver:http")

// io.opentelemetry.sdk.extension.incubator.fileconfig
implementation("com.fasterxml.jackson.core:jackson-databind")
implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-yaml")
Copy link
Member

Choose a reason for hiding this comment

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

Will this eventually make jackson a hard dependency for the sdk or just this extension?

Copy link
Member

Choose a reason for hiding this comment

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

jackson-databind gets CVEs very often, I'd sleep better if we don't include it in the SDK (at some point); would jackson-jr be able to handle the generated POJOs?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would likely eventually be merged into the autoconfigure module, rather than the SDK itself.

I can investigate jackson-jr.

Also, there's the more hands on option of doing something like we do with YAML view configuration, where we use snake YAML to parse to a generic type, then extract the fields manually. Maybe we use jackson-databind / jackson-jr to prototype, and consider switching to the manual method when its looking more complete.

Copy link
Member

Choose a reason for hiding this comment

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

Considering this is for configuration, which is likely only done at startup I feel we should go with a potentially less performant option and not use jackson in exchange for fewer dependencies and CVE exposure.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's less of the performance I'm worried about and more the additional complexity to manually do the parsing that jackson does automatically. Additionally, there is likely to be a fair amount of churn in the early versions of the configuration schema - I'm happy enough writing manual parsing code once, but won't sign up to rewrite the manual parsing code for each of the early iterations.

sdk-extensions/incubator/build.gradle.kts Outdated Show resolved Hide resolved
implementation(project(":semconv"))
compileOnly("com.sun.net.httpserver:http")

// io.opentelemetry.sdk.extension.incubator.fileconfig
implementation("com.fasterxml.jackson.core:jackson-databind")
implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-yaml")
Copy link
Member

Choose a reason for hiding this comment

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

jackson-databind gets CVEs very often, I'd sleep better if we don't include it in the SDK (at some point); would jackson-jr be able to handle the generated POJOs?

@jack-berg
Copy link
Member Author

Alright it's been a while but I made a bunch of updates to this and I'd like us to seriously consider merging it. Notes:

  • There are now builders for the generated POJOs, which makes the tests much more readable and the pojos much easier to work with.
  • The builders generated by jsonSchema2Pojo plugin are a bit awkward and they produce "rawtypes" warnings. I had to do some gradle magic to annotate the generated classes with @SuppressWarnings("rawtypes" to make the compiler happy.
  • jsonSchema2Pojo is supposed to annotate generated classes with a version of @Generated (either javax.annotation.processing.* or javax.annotation.*) based on the java runtime used to build, but it behaves weird with our setup where we: use java 17 to build, have the release version set to java 8, and include both versions of the @Generated class. I had to workaround this by forcing jsonSchema2Pojo to add the java 9+ annotation, but then rewrite it to the java 8 version in a gradle task. It was quite annoying.

The opentelemetry-configuration schema has come a long way since I opened this PR and its in a place where I can actually start implementing the logic to interpret a parsed model and turn it into functional SDK elements. I will start doing that in followup PRs.

@jack-berg
Copy link
Member Author

@jkwatson any comments before I merge this?

@jkwatson
Copy link
Contributor

jkwatson commented Aug 3, 2023

:shipit:

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.

5 participants