Should AttributesExtractor be an interface?

[trask]

Only bringing up because all other Extractors are interfaces, and Intellij has had to correct me on extends and implements a few times because it took me some time to notice the difference.

Related, is the set method in AttributesExtractor needed? It looks like AttributesBuilder is no-op already on null value, or is that subject to change?

[anuraaga]

I thought about it a bit, but it made the instrumentation library deal with two types. Instead of addAttributesExtractor(new HttpAttributesExtractor()) it would need to be something like addAttributesExtractor(HttpAttributesExtractor.create(new HttpAttributesGetter())). Seemed like a net loss but open to it if more implements instead of extends makes it worth it for others, I do feel the pain too.

[anuraaga]

Well - maybe there's addAttributesGetter(new HttpAttributesGetter()), and in our code, we do instanceof to determine the right extractor. Didn't look into that but it could be ok, we'd still need to keep addAttributesExtractor for custom extractors though

[trask]

I'm not following how that's related to AttributesExtractor being an interface or abstract class? (I added to Wed meeting agenda in case it's easier to explain then)

[anuraaga]

@trask We embed the logic of mapping getter methods to semantic attributes in the abstract classes, if an interface we'd need to move it somewhere, or make them public which I think we should avoid since these are implementations of spec and shouldn't be customized. I thought moving them requires having another class the user knows about with the mapping logic, but #3131 (comment) makes me think there's another way around it.

[trask]

oh I see, I was only thinking of making the one class AttributesExtractor into an interface. HttpAttributesExtractor sort of makes sense to me as an abstract base class.

the attributes getter idea sounds interesting (not mixing extractor + getter), what's your thought on addAttributesExtractor overloads to make it clear that it's only for official semantic convention extractors?

[iNikem]

From the consistency point of view I support changing just io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor from an abstract class to an interface. WDYT? If we have enough votes, I can do it.

[anuraaga]

@iNikem I'm still quite not in favor of simply changing all the methods to public

#3131 (comment)

Are you thinking of taking the split extractor / getter approach?

[iNikem]

@anuraaga No, I am thinking about converting only 1 class, io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor. It will have only 2 public methods, onStart and onEnd. I don't see any problems with that. Or what do I missing?

[anuraaga]

Ah ok, yeah if the semantic attribute extractors are still classes it seems fine. Oops, I might have misinterpreted @trask's original issue too to mean all of them, not just the supertype :)