Handle kafka METRIC_REPORTER_CLASSES_CONFIG being set to a List#9155
Conversation
mateuszrzeszutek
left a comment
There was a problem hiding this comment.
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.
can you add a comment, I couldn't figure out what this check is for, thx
There was a problem hiding this comment.
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.
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.
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_CONFIGvalid 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.