http server and client builder for netty #12083
Conversation
zeitlinger
changed the title
| public String getInstrumentationName() { | ||
| return instrumentationName; | ||
| } |
There was a problem hiding this comment.
to me this method and getOpenTelemetry feel out of place in this class.
There was a problem hiding this comment.
added a instrumenterBuilder method instead and only exposed propagators of OpenTelemetry
...try/instrumentation/api/incubator/builder/internal/DefaultHttpServerInstrumenterBuilder.java
Outdated
Show resolved
Hide resolved
...y/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterBuilderFactory.java
Outdated
Show resolved
Hide resolved
...y/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyServerTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
test native
|
@laurit please check again |
| // this logic is only used in agent | ||
| builder.setEmitExperimentalHttpServerEvents(true); |
There was a problem hiding this comment.
is there reason not to expose this to library instrumentation?
There was a problem hiding this comment.
just for javaagent, there is no dedicated property
...ain/java/io/opentelemetry/javaagent/bootstrap/internal/JavaagentHttpServerInstrumenters.java
Outdated
Show resolved
Hide resolved
| public <BUILDERREQUEST, BUILDERRESPONSE> | ||
| InstrumenterBuilder<BUILDERREQUEST, BUILDERRESPONSE> instrumenterBuilder( | ||
| SpanNameExtractor<? super BUILDERREQUEST> spanNameExtractor) { | ||
| return Instrumenter.builder(openTelemetry, instrumentationName, spanNameExtractor); |
There was a problem hiding this comment.
this method is confusing, I finally understood it, but wonder if there's anything we can do to make it clearer
There was a problem hiding this comment.
maybe toInstrumenterBuilder?
| if (headerGetter != null) { | ||
| return builder.buildServerInstrumenter(headerGetter); | ||
| } | ||
| return builder.buildInstrumenter(SpanKindExtractor.alwaysServer()); |
There was a problem hiding this comment.
are there any server instrumentations that don't provide a headerGetter?
There was a problem hiding this comment.
made it part of the ctor
There was a problem hiding this comment.
just found that ktor doesn't have the header getter - but this is an upcoming pr
|
@trask please check again |
...try/instrumentation/api/incubator/builder/internal/DefaultHttpClientInstrumenterBuilder.java
Outdated
Show resolved
Hide resolved
...try/instrumentation/api/incubator/builder/internal/DefaultHttpServerInstrumenterBuilder.java
Outdated
Show resolved
Hide resolved
…tors of `OpenTelemetry`
zeitlinger
force-pushed
the
http-server-builder-netty
branch
from
fdcbbea to
eb0cc91
Compare
|
@laurit please check again |
No description provided.