Split ArmeriaTelemetry into client and server #12851
Conversation
github-actions
bot
added
the
test native
trask
force-pushed
the
split-armeria
branch
3 times, most recently
from
71e277c to
4c95169
Compare
trask
force-pushed
the
split-armeria
branch
3 times, most recently
from
ed75537 to
624a400
Compare
| import java.util.function.Function; | ||
|
|
||
| /** Entrypoint for instrumenting Armeria clients. */ | ||
| public final class ArmeriaClientTelemetry { |
There was a problem hiding this comment.
ktor has server and client instrumentations in separate packages, should we do the same here or change ktor?
There was a problem hiding this comment.
since they're small packages and we'd probably want to keep "Client" and "Server" in the class names not to conflict anyways, I think I'm leaning towards single package (and changing ktor)
There was a problem hiding this comment.
one benefit for having a single package could be that when you initially have only server instrumentation, like in ktor-1, then adding client instrumentation wouldn't force changing the package (unless the author had the foresight to use the correct package from the start).
...c/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaClientTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaServerTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
| @@ -21,13 +21,13 @@ class ArmeriaHttpClientTest extends AbstractArmeriaHttpClientTest { | |||
| @Override | |||
| protected WebClientBuilder configureClient(WebClientBuilder clientBuilder) { | |||
| return clientBuilder.decorator( | |||
| ArmeriaTelemetry.builder(testing.getOpenTelemetry()) | |||
| .setCapturedClientRequestHeaders( | |||
| ArmeriaClientTelemetry.builder(testing.getOpenTelemetry()) | |||
There was a problem hiding this comment.
WDYT should we also have a test for the deprecated telemetry class?
There was a problem hiding this comment.
sounds reasonable, I'll make a copy
ArmeriaTelemetry*classes have been deprecated and split intoArmeriaClientTelemetry*andArmeriaServerTelemetry*Part of #12846
See #12867 for change log migration notes entry