add telemetry to GCS #334
add telemetry to GCS #334
Conversation
| return null; | ||
| } | ||
| try { | ||
| return getClass(GCS_METRICS).asSubclass(ApiTracerFactory.class).getDeclaredConstructor().newInstance(); |
There was a problem hiding this comment.
I see that not all implementations of ApiTracerFactory have a default constructor that you are using here. So I'm just curious what implementation is supposed to be used here?
There was a problem hiding this comment.
I see that not all implementations of ApiTracerFactory have a default constructor
I was going to use a non open source one.
Happy to add a more complex builder mechanism, with some standard implementations for suggested Tracers
| try { | ||
| return getClass(GCS_METRICS).asSubclass(ApiTracerFactory.class).getDeclaredConstructor().newInstance(); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Can it be ConfigException with the message clarifying why the instantiation of ApiTracerFactory failed and which class is expected there?
There was a problem hiding this comment.
Done. Just my lazyness - its just that ConfigException doesn't follow normal exception chaining patterns
| /* | ||
| * Copyright 2024 Aiven Oy | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package io.aiven.kafka.connect.common.config.validators; | ||
|
|
||
| import org.apache.kafka.common.config.ConfigDef; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
|
|
||
| public class ClassValidator implements ConfigDef.Validator { | ||
| private final Class<?> baseClass; | ||
|
|
||
| public ClassValidator(final Class<?> baseClass) { | ||
| this.baseClass = baseClass; | ||
| } | ||
|
|
||
| @Override | ||
| public void ensureValid(final String name, final Object value) { | ||
| if (value != null && !baseClass.isAssignableFrom((Class<?>) value)) { | ||
| throw new ConfigException(name, value, "must be a subclass of " + baseClass.getName()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we instead bring this validator implementation here? IMO it makes code a little more readable when you write Subclass.of(SomeClass.class) rather then new ClassValidator(SomeClass.class)
There was a problem hiding this comment.
Is there a common library that you use for these. It seems copying and duplication is a bad way forward, particularly for a common concept like validation
There was a problem hiding this comment.
There is no common library yet and when I created the validator I'm suggesting I did not manage to find a ready to use implementation.
I think for now copying is good enough but in future I think if this pattern will repeat we will consider creating our own commons library to address this.
mostly so that you can use the google supplied telemetry