NetClientAttributesAdapter - favor composition over inheritance - #5030
Conversation
|
hey @breedx-splk! there was some related discussion in #3131 it didn't seem like having the instrumentation library deal with two types was worth it (e.g. in this case NetClientAttributesExtractor and NetClientAttributesAdapter), though @anuraaga had a thought to mitigate that also, if we decide to do this for Net(Client|Server)AttributesExtractor, I assume we would do it for Http(Client|Server)AttributesExtractor, DbAttributesExtractor, CodeAttributesExtractor, RpcAttributesExtractor, and MessagingAttributesExtractor too? |
Thanks, I didn't realize this had come up before.
I dunno, I think it's worth it. Maybe it's worth getting @anuraaga to chime in again.
I think definitely that would be the longer term goal....to be consistent in eliminating inheritance as an api mechanism. |
|
@breedx-splk we discussed in yesterday's SIG and there is support for this change 👍 one suggestion is to rename |
53aafff to
5061002
Compare
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
can you do one more pass through to change remaining adapter to getter terminology?
|
I think I got them all now. |
|
go to https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5030/files and search for |
Ah, lots of local variable names that didn't make it through the rename. I'll get out the mop.... |
…lientAttributesExtractor concrete
781fffb to
42d2122
Compare
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
there's a couple remaining references to "adapter" term, if you don't mind updating those too, then let's get this merged!
…-telemetry#5030) * first pass at separating NetAttributesAdapter interface and make NetClientAttributesExtractor concrete * rename the implementations extractor -> adapter * hide constructor and make factory method * rename to client and add javadoc * spotless * finish javadoc thought * rebase * renamed NetClientAttributesAdapter to NetClientAttributesGetter * fix lettuce * code review comments * code review comments -- renaming for consistency * adapter -> getter * fix ratpack * adapter -> getter
3 participants
The goal here is to reduce the number of responsibilities in the
NetClientAttributesExtractor(single responsibility principle) and to favor composition/delegation over inheritance. To do this, we extracted the abstract methods to an interface namedNetClientAttributesAdapter, which theNetClientAtrributesExtractordelegates to. This allows it to now be made concrete (and final). The existing implementations are migrated to the new interface instead of extendingNetClientAttributesAdapter.To reduce the scope of this change (which already touches a lot of code), this does not include a similar change to the
NetServerAttributesExtractor, but I think we should have parity via a follow-up PR.