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

Remove deprecated code #6501

Merged
merged 3 commits into from
Aug 24, 2022
Merged

Conversation

mateuszrzeszutek
Copy link
Member

No description provided.

@mateuszrzeszutek mateuszrzeszutek requested a review from a team August 23, 2022 13:38
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.

Looks good. Distributions that haven't addressed the deprecation will have some exciting work ahead, huh? 🌶️ Anyway, deleting deprecated code is awesome!

AutoConfiguredOpenTelemetrySdkBuilder builder =
AutoConfiguredOpenTelemetrySdk.builder()
.setResultAsGlobal(true)
.addPropertiesSupplier(config::getAllProperties);
.addPropertiesSupplier(new ConfigurationFileLoader());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.addPropertiesSupplier(new ConfigurationFileLoader());
.addPropertiesSupplier(ConfigurationFileLoader::new);

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't that result in a Supplier<Supplier<Map<String, String>>>?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're right. Ignore me.

static final String CONFIGURATION_FILE_PROPERTY = "otel.javaagent.configuration-file";

@Override
public Map<String, String> get() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a fair amount going on in this method. I think it would be helpful to have some test coverage...but the current structure is a bit harder to test due to the static calls to ConfigPropertiesUtil.getString() and the inline creation of file streams. Still, there are at least 5 untested branches....

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 is actually tested, in ConfigurationFileTest.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

so nice to delete all of the config classes!

@trask trask merged commit 59cb9ca into open-telemetry:main Aug 24, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the remove-deprecations branch August 25, 2022 07:46
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
* Remove deprecated code

* unnecessary semicolon

* fix distro and extension examples
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
* Remove deprecated code

* unnecessary semicolon

* fix distro and extension examples
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
* Remove deprecated code

* unnecessary semicolon

* fix distro and extension examples
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