-
Notifications
You must be signed in to change notification settings - Fork 791
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
Handle kafka METRIC_REPORTER_CLASSES_CONFIG being set to a List #9155
Conversation
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.
Is it possible for METRIC_REPORTER_CLASSES_CONFIG
to be a single Class<?>
?
not possible, also using a set instead of a list failed. |
|| config.get(OpenTelemetryMetricsReporter.CONFIG_KEY_OPENTELEMETRY_INSTRUMENTATION_NAME) | ||
!= null) { |
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.
can you add a comment, I couldn't figure out what this check is for, thx
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.
this is for ensuring that we don't try to modify the same configuration twice, added a comment
if (METRIC_REPORTER_PRESENT_PATTERN.matcher(className1).find()) { | ||
return class1; | ||
} |
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.
Not 100% sure of the original intent, but it sure looked like the pattern was here to attempt to prevent duplicates. Removing this now allows duplicates, which could possibly allow for duplicated telemetry (unless there's some other hidden guardrails in place).
In any case, this is a change in behavior. Looks like there wasn't test coverage anyway, so .........
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.
There is a test for this. Duplicates are now handled with config.get(OpenTelemetryMetricsReporter.CONFIG_KEY_OPENTELEMETRY_INSTRUMENTATION_NAME) != null
check at the start of the method as outlined in the pr description
Resolves #9143
For
METRIC_REPORTER_CLASSES_CONFIG
valid values seem to be a comma separated string with class names, list of classes and list of class names. Instead of checking with regex whether our metrics reporter has been added we'll now check whether instrumentation name has been added to config and is so skip enhancing. Duplicate enhancement happens because we do it in the constructors of producer/consumer and due to constructor chaining it may be called multiple times.