Library readmes - batch 2 #14631
Library readmes - batch 2 #14631
Conversation
| import io.opentelemetry.api.OpenTelemetry; | ||
| import io.opentelemetry.instrumentation.jmx.engine.JmxMetricInsight; | ||
| import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration; | ||
|
|
||
| // Get an OpenTelemetry instance | ||
| OpenTelemetry openTelemetry = ...; | ||
|
|
||
| JmxMetricInsight jmxMetricInsight = JmxMetricInsight.createService(openTelemetry, 5000); | ||
|
|
||
| // Configure your JMX metrics | ||
| MetricConfiguration config = new MetricConfiguration(); | ||
|
|
||
| jmxMetricInsight.startLocal(config); |
There was a problem hiding this comment.
Unfortunately this does not conform to our usual practices. @SylvainJuge any plans to make this similar to our other library instrumentations?
There was a problem hiding this comment.
What do you mean exactly here ? Is it packaging the JMX metrics feature to be similar to other library components in the instrumentation repository ? For example support for auto-configuration or fitting an "idiomatic way" to setup and initialize ?
For example, there is currently no way to properly shut-down or disable the feature at runtime, which could be problematic in some case if we want to provide the ability to automatically re-connect to a remote JVM (which is not the case when executed locally within the JVM).
There was a problem hiding this comment.
Other library instrumentations use have Telemetry/TelemetryBuilder as public api classes that users should interact with, idk if the same pattern would work here. Our library instrumentations provide minimal public api, this module has a bunch of public classes, are all of these intended to serve as the public api? If not you could move them to an internal package. The main package is io.opentelemetry.instrumentation.jmx.engine which does not match what other library insrumentations use.
| .addLast(clientTelemetry.createRequestHandler()) | ||
| .addLast(clientTelemetry.createResponseHandler()); |
There was a problem hiding this comment.
could use createCombinedHandler() to create both
| NettyClientTelemetry clientTelemetry = NettyClientTelemetry.create(openTelemetry); | ||
|
|
||
| Bootstrap bootstrap = new Bootstrap(); | ||
| bootstrap.group(new NioEventLoopGroup()) |
There was a problem hiding this comment.
idk if that matters but NioEventLoopGroup has been deprecated in netty 4.2
3 participants
Contributes to #6947