Skip to content

Idea: deprecate Config, add agent-only InstrumentationConfig#6264

Merged
mateuszrzeszutek merged 4 commits intoopen-telemetry:mainfrom
mateuszrzeszutek:instrumentation-config
Jul 8, 2022
Merged

Idea: deprecate Config, add agent-only InstrumentationConfig#6264
mateuszrzeszutek merged 4 commits intoopen-telemetry:mainfrom
mateuszrzeszutek:instrumentation-config

Conversation

@mateuszrzeszutek
Copy link
Copy Markdown
Member

... and bridge InstrumentationConfig calls to SDK's ConfigProperties. I tried drafting something that implements ideas from #6250, i.e.: making Config/InstrumentationConfig an internal, agent-only class.

@mateuszrzeszutek mateuszrzeszutek requested a review from a team July 5, 2022 10:12
@mateuszrzeszutek mateuszrzeszutek marked this pull request as draft July 5, 2022 10:13
Comment on lines +190 to +191
String value = config.getString("otel.instrumentation.experimental.span-suppression-strategy");
if (value != null) {
System.setProperty("otel.instrumentation.experimental.span-suppression-strategy", value);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If there are distros setting the default value of this, we need to copy the value over to system properties so that instrumentation-api code can get the correct value.

Comment on lines -21 to -29
/**
* TraceConfig Instrumentation does not extend Default.
*
* <p>Instead it directly implements Instrumenter#instrument() and adds one default Instrumenter for
* every configured class+method-list.
*
* <p>If this becomes a more common use case the building logic should be abstracted out into a
* super class.
*/
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I found a super outdated comment 😄

@Override
public void configure(Map<String, ?> configs) {}
public void configure(Map<String, ?> configs) {
// TODO: support experimental attributes config
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Later, in a separate PR, we should refactor this classes so that they use a similar way to configure themselves as introduced in #6138 -- it seems to be the "idiomatic" approach for kafka extensions, I guess

Comment on lines +24 to +25
ConfigPropertiesUtil.getBoolean(
"otel.instrumentation.aws-sdk.experimental-span-attributes", false))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

library-autoconfigure modules have to keep on using env var/system prop configuration, as it's their only way of configuring themselves.

Copy link
Copy Markdown
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.

This seems like a good approach until we have an opportunity to do something more radical with autoconfigure's config

@mateuszrzeszutek mateuszrzeszutek marked this pull request as ready for review July 6, 2022 07:10
@mateuszrzeszutek
Copy link
Copy Markdown
Member Author

Okay, I'll mark this PR as ready for review. There's quite a lot of changes to be done to deprecate Config, I'm going to split them across several PRs to make them easier to digest.

Copy link
Copy Markdown
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.

👍

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