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

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

Merged

Conversation

mateuszrzeszutek
Copy link
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
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
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 😄

@@ -34,5 +34,7 @@ public void onCommit(Map<TopicPartition, OffsetAndMetadata> offsets) {}
public void close() {}

@Override
public void configure(Map<String, ?> configs) {}
public void configure(Map<String, ?> configs) {
// TODO: support experimental attributes config
Copy link
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
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
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
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
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