Closes #1297 - Add exporter services and their registration with OpenTelemetry #1375
Conversation
…r switch metrics.enabled
- renamed "actions" to "action" (singular instead of plural) - only the "inspectit/self/action/execution-time" is recorded. The execution count is computed as a COUNT aggregation in the view - fixed the ActionRecorderTestAgent to test whether the metrics are recorded properly with IActionHooks - changed the settings for ActionMetricsRecorder to ActionMetricsRecorderSettings (new class) - changed some "val" to typed variables
…1-self-monitoring
…oved unused reference to ActionMetricsRecorder in MethodHook and MethodHookGenerator.
…e, and added BoundGenericAction#toString that returns the action's name and the name of the call.
… opencensus-shim), see inspectIT#1246. Implemented a LoggingExporterService that exports traces to the LoggingSpanExporter and metrics to the LoggingMetricExporter.
…ces and metrics into two separate classes LoggingMetricExporterService and LoggingTraceExporterService.
OpenTelemetry guarantees Java 8 support and higher Jackson versions are imported with OTel
…te-oc-otel-bridge # Conflicts: # gradle.properties # inspectit-ocelot-core/build.gradle
…t and LoggingTraceExporterServiceTest
…try and the OpenCensus-Shim (inspectIT#1246).
…31-self-monitoring
…t and io.grpc.Context in ContextManager and InspectitContextImpl and implemented the helper class ContextUtil. With this approach, we aim at supporting the transition from OpenCensus to OpenTelemetry with the io.opentelemetry.opencensusshim. While OpenTelemetry uses io.opentelemetry.context.Context, OpenCensus uses io.grpc.Context, and some custom features of inspectIT also work with io.grpc.Context (inspectIT#1246)
…tus to OTEL StatusData
…migration' into features/feat-1297-integrate-jaeger-proto-and-otlp-exporters
|
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml, line 55 at r5 (raw file):
What should be the default value for
|
|
You can find the most important changes as well as an architectural description in the attached file 2022-01-11-ticket-1269-1279-otel-exporters-otel-upgrade-changenotes.pdf |
…tomatically derive `protocol` from `url`/ `grpc` in the `JaegerExporterSettings` for this release
There was a problem hiding this comment.
Reviewed 7 of 78 files at r4, 91 of 120 files at r5, 4 of 8 files at r6, all commit messages.
Reviewable status: 102 of 127 files reviewed, 69 unresolved discussions (waiting on @heiko-holz and @mariusoe)
components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java, line 47 at r6 (raw file):
@ConditionalOnExpression("(NOT new String('${inspectit-eum-server.exporters.tracing.jaeger.enabled}').toUpperCase().equals(T(rocks.inspectit.ocelot.config.model.exporters.ExporterEnabledState).DISABLED.toString())) AND (new String('${inspectit-eum-server.exporters.tracing.jaeger.endpoint}').length() > 0)") public SpanExporter jaegerSpanExporter() { @Valid JaegerExporterSettings jaegerExporterSettings = configuration.getExporters().getTracing().getJaeger();
Is this inline annotation having an effect at all?
Code quote:
@Valid inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java, line 4 at r6 (raw file):
import lombok.AllArgsConstructor; import lombok.Value;
Revert please - don't needed.
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/logging/Log4J2TraceIdAutoInjectorTest.java, line 36 at r6 (raw file):
@Test public void logStringAndTraceExists() { System.out.println(String.format("OTEL=%s, tracer=%s", Instances.openTelemetryController.getClass()
Use the LOGGER for logging
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/utils/TestUtils.java, line 238 at r6 (raw file):
* Waits for the initialization of {@link rocks.inspectit.ocelot.core.opentelemetry.OpenTelemetryControllerImpl}, which is then registered to {@link Instances#openTelemetryController} */ public static void waitForOpenTelemetryControllerInitialization() {
As far as I can see, this is not used.
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/utils/TestUtils.java, line 240 at r6 (raw file):
public static void waitForOpenTelemetryControllerInitialization() { Awaitility.await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> { assertThat(NoopOpenTelemetryController.INSTANCE != Instances.openTelemetryController).isTrue();
assertThat(Instances.openTelemetryController).isNotEqualTo(NoopOpenTelemetryController.INSTANCE);
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java, line 6 at r6 (raw file):
* Controller interface for the OpenTelemetryController. Its implementation is {@link rocks.inspectit.ocelot.core.opentelemetry.OpenTelemetryControllerImpl} */ public interface IOpenTelemetryController {
Use just OpenTelemetryController. We don't use the I prefix because the implementation is suffixed with Impl. Imo we should not use both "markers".
Code quote:
IOpenTelemetryControllerinspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java, line 13 at r6 (raw file):
* @return */ static IOpenTelemetryController noop() {
Not used and needed. We can directly use the Noop instance with NoopOpenTelemetryController.INSTANCE when needed.
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java, line 22 at r6 (raw file):
* @return Whether the {@link IOpenTelemetryController} is configured */ boolean isConfigured();
Is this only used for testing?
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java, line 29 at r6 (raw file):
* @return Whether the {@link IOpenTelemetryController} is enabled */ boolean isEnabled();
In my opinion, the names are not unique as to what the return value indicates.
At first glance, we have the following "states":
- enabled
- configured
- started
- stopped
- shutdown
But as far as I can see in the code, enable is actually equal to started and configured, right? And stopped means that the controller has been shut down.
Maybe we can use something like:
- isStarted()
- start()
- has/isShutdown()
- shutdown()
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java, line 29 at r6 (raw file):
* @return Whether the {@link IOpenTelemetryController} is enabled */ boolean isEnabled();
Is this only used for testing?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/StringToTransportProtocolConverter.java, line 14 at r6 (raw file):
@Override public TransportProtocol convert(String source) { return StringUtils.hasText(source) ? TransportProtocol.parse(source) : TransportProtocol.UNSET;
Would it be sufficient when we use UNSET as default value and let Spring handle the conversion instead of manually doing more or less the same?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java, line 12 at r6 (raw file):
*/ public enum TransportProtocol { UNKNOWN("unknown"), UNSET(""), GRPC("grpc"), HTTP_THRIFT("http/thrift"), HTTP_PROTOBUF("http/protobuf"), HTTP_JSON("http/json"), COUNT("cnt");
Why do we need UNKNOWN, HTTP_JSON and COUNT
Please only add those which are needed - or do you have some PR in your pipeline which is needing these options?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java, line 37 at r6 (raw file):
*/ public static TransportProtocol parse(String name) { return names.containsKey(name) ? names.get(name) : TransportProtocol.valueOf(name);
Why do you do this custom look-up and don't let it handled by spring?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/InfluxExporterSettings.java, line 34 at r6 (raw file):
* The HTTP URL endpoint of Influx (e.g., http://localhost:8086) */ private String endpoint;
See comment in InfluxExporterService
-> depending whether we support InfluxDB in the future, we should already add a deprecation warning or something like that.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/OtlpMetricsExporterSettings.java, line 35 at r6 (raw file):
* Supported protocols are {@link TransportProtocol#GRPC} and {@link TransportProtocol#HTTP_PROTOBUF} */ private TransportProtocol protocol;
Since TransportProtocol contains more values than can be used here, we should directly validate that a supported value is set here.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/JaegerExporterSettings.java, line 38 at r6 (raw file):
* Supported protocols are {@link TransportProtocol#HTTP_THRIFT} and {@link TransportProtocol#GRPC} */ private TransportProtocol protocol;
Since TransportProtocol contains more values than can be used here, we should directly validate that a supported value is set here.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/JaegerExporterSettings.java, line 43 at r6 (raw file):
* The service name. Used in {@link rocks.inspectit.oce.eum.server.exporters.configuration.TraceExportersConfiguration} */ private String serviceName;
Since many exporters are using the inspectit.service-name attribute directly, we should consider to drop this attribute on individual exporter setting so it is consistent.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/OtlpTraceExporterSettings.java, line 21 at r6 (raw file):
* This property is deprecated since v2.0. Please use {@link #endpoint} instead. * The OTLP traces gRPC endpoint to connect to. */ private String url;
Imo this cannot happen that URL is set because until now, there are not OTLP settings. So url can be removed.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/OtlpTraceExporterSettings.java, line 32 at r6 (raw file):
* Supported protocols are {@link TransportProtocol#GRPC} and {@link TransportProtocol#HTTP_PROTOBUF} */ private TransportProtocol protocol;
Since TransportProtocol contains more values than can be used here, we should directly validate that a supported value is set here.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/tracing/TracingSettings.java, line 63 at r6 (raw file):
private PropagationFormat propagationFormat = PropagationFormat.B3; static final int DEFAULT_MAX_EXPORT_BATCH_SIZE = 512;
As far as I can see, there is no benefit for using a seperate static field for the default value. Imo it makes the code much more complicated and harder to maintain.
Keep it simple and consinsten and move the default values directly to the actual field or/and add them to the default configuration file, so the configuration file can used as a sample for all default values of the configuration.
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml, line 55 at r5 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
What should be the default value for
protocol?
We can either usenullso that theprotocoland theendpointneed to be specified in order to start the exporter, or we could use the following default values:
- jaeger:
http/thrift- otlp:
grpc
How about using UNSET as default value?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/spring/BootstrapInitializerConfiguration.java, line 60 at r6 (raw file):
@Bean(OpenTelemetryControllerImpl.BEAN_NAME) public OpenTelemetryControllerImpl getOpenTelemetryController(InspectitEnvironment environment) { InspectitConfig configuration = environment.getCurrentConfig();
Not needed
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/DynamicallyActivatableMetricsExporterService.java, line 21 at r6 (raw file):
* @return A new {@link MetricReaderFactory} */ public abstract MetricReaderFactory getNewMetricReaderFactory();
Similar to the comment in the DynamicallyActivatableTraceExporterService class.
We can refactor the method registerMetricExporterService(DynamicallyActivatableMetricsExporterService service) of the OpenTelemetryController to the following:
registerMetricExporterService(MetricReaderFactory factory, String name)
then we don't need this class and can directly use the DynamicallyActivatableService which makes it simple in my oppinion.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/DynamicallyActivatableTraceExporterService.java, line 12 at r6 (raw file):
* This class extends {@link DynamicallyActivatableService} which handles the waiting for changes in the configuration. */ public abstract class DynamicallyActivatableTraceExporterService extends DynamicallyActivatableService {
I don't get the point why we need this class. The only time getSpanExporter is used is during registration in the OpenTelemetryCollector#registerTraceExporterService where an instance of this class is passed.
We could also refactor that method from
registerTraceExporterService(DynamicallyActivatableTraceExporterService service)
to
registerTraceExporterService(SpanExporter exporter, String name)
then we don't need this class and can just use DynamicallyActivatableService
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java, line 24 at r6 (raw file):
@Slf4j @Component public class InfluxExporterService extends DynamicallyActivatableService {
Is this exporter still working? It is based on OpenCensus, right. It will stop working with the final migration from Bridge to OpenTelemetry - I guess..
Depending whether we provide an InfluxDB exporter for OpenTelemetry (which I would not recommend), we should discuss dropping support for direct InfluxDB metric exportation. As an alternative, the OTel collector can be used.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingMetricExporterService.java, line 57 at r6 (raw file):
boolean success = openTelemetryController.registerMetricExporterService(this); if (success) { log.info("Starting {}", getClass().getSimpleName());
getName()
Code quote:
getClass().getSimpleName()inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingMetricExporterService.java, line 59 at r6 (raw file):
log.info("Starting {}", getClass().getSimpleName()); } else { log.error("Failed to register {} at {}!", getClass().getSimpleName(), openTelemetryController.getClass()
Would it not be sufficient to log:
log.error("Failed to register {} at the OpenTelemetry controller!", getName());
Code quote:
log.error("Failed to register {} at {}!", getClass().getSimpleName(), openTelemetryController.getClass()
.getSimpleName());inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingTraceExporterService.java, line 51 at r6 (raw file):
boolean success = openTelemetryController.registerTraceExporterService(this); if (success) { log.info("Starting {}", getClass().getSimpleName());
See comments in the LogginmetricExporterService
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/PrometheusExporterService.java, line 35 at r6 (raw file):
@Override protected boolean doEnable(InspectitConfig configuration) { val config = configuration.getExporters().getMetrics().getPrometheus();
Can you replace this val with the actual class
Code quote:
valinspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/ZipkinExporterService.java, line 55 at r6 (raw file):
spanExporter = ZipkinSpanExporter.builder().setEndpoint(endpoint).build(); // register
Please remove this inline comments
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/context/ContextUtil.java, line 46 at r6 (raw file):
* @return The {@link InspectitContext} stored in the {@link #currentGrpc() current GRPC context} with the {@link InspectitContextImpl#INSPECTIT_KEY_GRPC} */ public static InspectitContextImpl currentInspectitContextStoredInGrpcContext() {
Not used.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHook.java, line 16 at r6 (raw file):
import rocks.inspectit.ocelot.core.selfmonitoring.IActionScope; import javax.validation.constraints.NotNull;
Revert please.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 156 at r6 (raw file):
@Order(Ordered.LOWEST_PRECEDENCE) synchronized private void startAtStartup(ContextRefreshedEvent event) { // start and configure OTEL at when the ApplicationContext gets initialized
please remove this inline comment because the code itself is quite obvious :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 165 at r6 (raw file):
* @return Whether the {@link OpenTelemetryControllerImpl} is currently (re-)configuring tracing and metrics */ public boolean isConfiguring() {
Not used. Just implement and keep the code we need. Otherwise we have to maintain dead code which is not good.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 179 at r6 (raw file):
@Order(Ordered.LOWEST_PRECEDENCE) @VisibleForTesting // make sure this is called after the individual services have (un)-registered
Put this comment into the Javadoc
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 193 at r6 (raw file):
InspectitConfig configuration = env.getCurrentConfig(); // set serviceName
Try to reduce the amount of inlince comments. Inline comments should usually be rather avoided to keep the code small and readable. If many inline comments are needed, it often indicates that there is something wrong with the code, e.g. that it is too complex or too large, or that parts of methods should perhaps be outsourced in order to keep a method small and simple (keyword: single responsiblity).
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 216 at r6 (raw file):
.setMeterProvider(sdkMeterProvider) .build(); // update OTEL
please remove this inline comment because the code itself is quite obvious :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 220 at r6 (raw file):
} success = null != sdkMeterProvider && null != sdkTracerProvider; // update meterProvider and tracerProvider
please remove this inline comment because the code itself is quite obvious :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 234 at r6 (raw file):
isConfiguring.set(false); // reset changed variables
please remove this inline comment because the code itself is quite obvious :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 242 at r6 (raw file):
@Override synchronized public boolean start() { enabled = true;
Is it correct that this can be set to true even the controller is not started? Is enabled not more or less the same as configured?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 244 at r6 (raw file):
enabled = true; // if OTEL has not been configured (since last shutdown), configure it return configured = !configured ? configureOpenTelemetry() : true;
Please prevent an assignment and return value within a single line because it is quite hard to get what is happening here.
Furthermore, is it ok to return true in case the controller has already been started? It could be confusing in case the method returns true but the controller has not been started at all because it is already running. How about an exception?
How about using something like this? It quite easy to read and you can easily see what is happening:
if (configured) {
throw new IllegalStateException("The controller is already running and cannot be started again.");
} else {
configured = configureOpenTelemetry();
return configured;
}
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 248 at r6 (raw file):
/** * Flushes the all pending spans ({@link #openTelemetry}) and metrics ({@link #meterProvider}) and waits for it to complete.
Flushes all pending...
Code quote:
Flushes the all pending inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 266 at r6 (raw file):
return; } if (!isShuttingDown.compareAndSet(false, true)) {
This can/should never happen due to the method synchronization.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 267 at r6 (raw file):
} if (!isShuttingDown.compareAndSet(false, true)) { log.info("Multiple shutdown calls");
Do we need some exception or returncall here? Now, the shutdown will be execute even there are multiple shutdown calls.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 318 at r6 (raw file):
.getServiceName())); // build new SdkTracerProvider
the building of the tracer provider can be extracted into a separate method, the you don't need all these inline comments because the methods are much smaller
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 335 at r6 (raw file):
tracerProvider = builder.build(); // build new SdkMeterProvider
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 346 at r6 (raw file):
// build and register OpenTelemetryImpl openTelemetry = OpenTelemetryImpl.builder().openTelemetry(openTelemetrySdk).build(); // check if any OpenTelemetry has been registered to GlobalOpenTelemetry.
Imo these comments are not needed.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 349 at r6 (raw file):
// If so, reset it. if (null != OpenTelemetryUtils.getGlobalOpenTelemetry()) { log.info("reset {}", GlobalOpenTelemetry.get().getClass().getName());
Can you use a more expressive log message
Code quote:
reset {}inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 371 at r6 (raw file):
} try { // update sample probability
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 389 at r6 (raw file):
*/ @VisibleForTesting synchronized SdkMeterProvider configureMeterProvider(InspectitConfig configuration) {
Argument not needed
Code quote:
InspectitConfig configurationinspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 397 at r6 (raw file):
// stop the previously registered MeterProvider if (null != meterProvider) { long start = System.nanoTime();
not needed
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 399 at r6 (raw file):
long start = System.nanoTime(); OpenTelemetryUtils.stopMeterProvider(meterProvider, true); // log.info("time to stopMeterProvider: {} ms", (System.nanoTime() - start) / 1000000);
Remove commented out logger code
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 401 at r6 (raw file):
// log.info("time to stopMeterProvider: {} ms", (System.nanoTime() - start) / 1000000); } // build new SdkMeterProvider
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 411 at r6 (raw file):
SdkMeterProvider sdkMeterProvider = builder.build(); return sdkMeterProvider;
return builder.build();
Code quote:
SdkMeterProvider sdkMeterProvider = builder.build();
return sdkMeterProvider;inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 422 at r6 (raw file):
* Registers a new {@link DynamicallyActivatableTraceExporterService} that is used to export {@link io.opentelemetry.sdk.trace.data.SpanData} for sampled {@link io.opentelemetry.api.trace.Span}s * * @param service
Please remove Javadoc code in case the documentation is not filled out, like this parameter or the return value of this method.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 427 at r6 (raw file):
*/ public boolean registerTraceExporterService(DynamicallyActivatableTraceExporterService service) { if (null == registeredTraceExportServices) {
This can never be null.. At least I don't see any call where it is set to null
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 468 at r6 (raw file):
private boolean unregisterTraceExporterService(String serviceName) { // unregister the service by removing it from the map of registered services and from the spanExporter // evaluates to true when a service with the given name was previously registered
Such information should be in the Javadoc instead as an inline comment
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java, line 497 at r6 (raw file):
*/ public boolean registerMetricExporterService(DynamicallyActivatableMetricsExporterService service) { if (null == registeredMetricExporterServices) {
This cannot be null.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 25 at r6 (raw file):
*/ @Slf4j @Builder
I would not use the builder pattern because there is only a single field to set.
I would recommend using the AllArgsConstructor annotation and makinglock final:
@AllArgsConstructor
public class OpenTelemetryImpl implements OpenTelemetry {
@NonNull
private OpenTelemetrySdk openTelemetry;
private final Object lock = new Object();
Then instead of the following
OpenTelemetryImpl.builder().openTelemetry(openTelemetrySdk).build();
it is just new OpenTelemetryImpl(openTelemetrySdk);.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 41 at r6 (raw file):
* @return */ public OpenTelemetry get() {
Can be private - or can be remove in case you add @NonNull to the openTelemetry field so it cannot be null at all
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 83 at r6 (raw file):
* @param openTelemetry The new {@link OpenTelemetrySdk} to register */ public void set(OpenTelemetrySdk openTelemetry) {
Not used, thus, can be removed
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 97 at r6 (raw file):
synchronized (lock) { if (null != openTelemetry) { // stop previous SdkTracerProvider if settings changed
Imo its fine to remove these comments, the code is quite expressive. And if you are very strict, this comment is not quite correct either, because the provider is stopped if the method parameter is true - which theoretically can be, if the settings haven't changed :P This is also one of the reasons why inline comments should rather be avoided, because you have to maintain not only the code but also the comments...
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 118 at r6 (raw file):
CompletableResultCode closeTracerProviderResultCode; CompletableResultCode closeMeterProviderResultCode; synchronized (lock) {
This lock would only make sense if we have multiple OpenTelemetryImpl instances. Is that possible?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 134 at r6 (raw file):
if (!resultCode.isDone()) { CountDownLatch latch = new CountDownLatch(1); resultCode.whenComplete(() -> latch.countDown());
You can pass a method reference directly:
resultCode.whenComplete(latch::countDown);
Code quote:
resultCode.whenComplete(() -> latch.countDown());inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 138 at r6 (raw file):
latch.await(15, TimeUnit.SECONDS); } catch (InterruptedException e) { e.printStackTrace();
Use some nice log message.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 149 at r6 (raw file):
*/ public OpenTelemetryImpl registerGlobal() { // register globally
please remove this comment
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java, line 151 at r6 (raw file):
// register globally GlobalOpenTelemetry.set(this); return this;
return value is never needed and can be removed
inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md, line 19 at r6 (raw file):
#### Added `OTLPMetricsExporter` and `OTLPTraceExporter` Due to the migration to OpenTelemetry, InspectIT Ocelot now supports OpenTelemetry Protocol (OTLP) exporter for metrics and tracing.
We use always an lower-case i for inspectIT
Code quote:
InspectITinspectit-ocelot-documentation/docs/tracing/tracing.md, line 32 at r6 (raw file):
|`max-export-batch-size`|512|The max export batch size for every export, i.e., the maximum number of spans exported by the used `BatchSpanProcessor`| |`schedule-delay-millis`|5000|The delay interval between two consecutive exports in milliseconds. **Note**: These properties take only effect once when the agent is starting. If you change these properties while the agent is running, they will not take effect until the agent retarted.
Move the "note" into a "warning block".
:::warning
These properties take only effect once when the agent is starting. If you change these properties while the agent is running, they will not take effect until the agent retarted.
:::
Then it is rendered like this one: https://inspectit.github.io/inspectit-ocelot/docs/configuration/agent-command-configuration
Code quote:
**Note**: These properties take only effect once when the agent is starting. If you change these properties while the agent is running, they will not take effect until the agent retarted.
There was a problem hiding this comment.
Reviewable status: 102 of 127 files reviewed, 69 unresolved discussions (waiting on @heiko-holz and @mariusoe)
components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java line 47 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Is this inline annotation having an effect at all?
That's a good point!
Let's discuss whether we need the @Valid annotations in any other class using the settings if they are already set in the MetricsExporterSettings.java and TraceExporterSettings.java
inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java line 4 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Revert please - don't needed.
Thanks for pointing this out. Unused imports are now removed (and thus the changes are reverted)
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/utils/TestUtils.java line 238 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As far as I can see, this is not used.
You are right. This method was used in previous version, but now it is not used anymore.
I removes this method.
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/utils/TestUtils.java line 240 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
assertThat(Instances.openTelemetryController).isNotEqualTo(NoopOpenTelemetryController.INSTANCE);
Done.
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 6 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Use just
OpenTelemetryController. We don't use theIprefix because the implementation is suffixed withImpl. Imo we should not use both "markers".
The I prefix is sometimes used, e.g., IGenericAction, IHookManager, IMethodHook.
Let's discuss this in our weekly.
-> create github issue for consistent interface naming
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 13 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not used and needed. We can directly use the Noop instance with
NoopOpenTelemetryController.INSTANCEwhen needed.
I agree. I was somewhat following the coding standards of OpenTelemetry but still references NoopOpenTelemetryController.INSTANCE.
What do you think is more suitable: dropping the IOpenTelemetry.noop() method (and being more in line with other implementations in inspectIT Ocelot) or keeping and using it instead of NoopOpenTElemetryController INSTANCE (aligning it to the code of OpenTelemetry)
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 22 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Is this only used for testing?
Originally, I wanted to implement a method that reliably tells whether the IOpenTelemetryController is configured and ready to run. The method is currently only used in test classes.
Howeve, I use the implementation @Getter private boolean configured in OpenTelemetryControllerImpl to decide whether OpenTelemetry ( OpenTelemetryImpl, SdkMeterProvider, SdkTracerProvider and OpenTelemetrySdk) needs to be configured.
Should I rather use the method isConfigured() in the implementation so that the usage can be detected more easily?
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 29 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
In my opinion, the names are not unique as to what the return value indicates.
At first glance, we have the following "states":
- enabled
- configured
- started
- stopped
- shutdown
But as far as I can see in the code, enable is actually equal to started and configured, right? And stopped means that the controller has been shut down.
Maybe we can use something like:
- isStarted()
- start()
- has/isShutdown()
- shutdown()
You are almost correct.
enabled equals started (which is not a state in my current implementation) and tells us whether the IOpenTelemetryController has started and is not stopped (shut down). It does not necessarily mean that it is already configured.
What do you think about the following:
- isStarted()
- start()
- isConfigured()
- isShutDown()
- shutdown()
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 29 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Is this only used for testing?
Same as with isConfigured(): I use the implementation @Getter private boolean stopped;
Should I rather use the isStopped() method instead of querying the stopped field?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/StringToTransportProtocolConverter.java line 14 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Would it be sufficient when we use
UNSETas default value and let Spring handle the conversion instead of manually doing more or less the same?
It depends on the values that we want to use in the configuration yaml.
I opted for the same wording as OpenTelemetry, which is grpc, http/thrift, http/protobuf, and http/json.
Due to the slash, I have to implement a custom parse that parses these strings into their enum representation.
To get rid of the StringToTransportProtocolConverter, we would need to change from using / in the protocols to _, which is not in line with the naming of OTEL.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java line 12 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Why do we need
UNKNOWN,HTTP_JSONandCOUNTPlease only add those which are needed - or do you have some PR in your pipeline which is needing these options?
I removed the unused HTTP_JSON.
Regarding UNKNOWN and COUNT, let's discuss this the weekly
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java line 37 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Why do you do this custom look-up and don't let it handled by spring?
See comment above.
I use this so that we can use / in the protocols to be in line with the naming of OTEL.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/OtlpMetricsExporterSettings.java line 35 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Since
TransportProtocolcontains more values than can be used here, we should directly validate that a supported value is set here.
Good point.
Can we implement a generic Validator for Enum?
As far as I am aware, we can only implement a specific Validator for specific enums, e.g., for TransportProtocol it would look like this:
/**
* Validate that an {@link TransportProtocol} is in the supported array of {@link #anyOf()}.
* Will fail if a property is not in {@link #anyOf()}.
*/
@Target({METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE})
@Retention(RUNTIME)
@Documented
@Constraint(validatedBy = TransportProtocolSubsetValidator.class)
public @interface TransportProtocolSubset {
TransportProtocol[] anyOf();
String message() default "must be any of {anyOf}";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
}/**
* Validator for {@link TransportProtocolSubset}
*/
public class TransportProtocolSubsetValidator implements ConstraintValidator<TransportProtocolSubset, Enum> {
private TransportProtocol[] subset;
@Override
public void initialize(TransportProtocolSubset constraintAnnotation) {
subset = constraintAnnotation.anyOf();
}
@Override
public boolean isValid(Enum value, ConstraintValidatorContext context) {
return Arrays.asList(subset).contains(value);
}
}
There was a problem hiding this comment.
Reviewable status: 102 of 127 files reviewed, 66 unresolved discussions (waiting on @heiko-holz and @mariusoe)
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 6 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
The
Iprefix is sometimes used, e.g.,IGenericAction,IHookManager,IMethodHook.Let's discuss this in our weekly.
-> create github issue for consistent interface naming
-> create github issue for consistent interface naming
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 13 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I agree. I was somewhat following the coding standards of OpenTelemetry but still references
NoopOpenTelemetryController.INSTANCE.What do you think is more suitable: dropping the
IOpenTelemetry.noop()method (and being more in line with other implementations in inspectIT Ocelot) or keeping and using it instead ofNoopOpenTElemetryController INSTANCE(aligning it to the code of OpenTelemetry)
As discussed, let's keep it consistent with the current code base for now.
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 29 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
You are almost correct.
enabledequalsstarted(which is not a state in my current implementation) and tells us whether theIOpenTelemetryControllerhas started and is notstopped(shut down). It does not necessarily mean that it is alreadyconfigured.What do you think about the following:
- isStarted()
- start()
- isConfigured()
- isShutDown()
- shutdown()
As discussed, let's rename isConfigured to isActive and rethink whether isStarted is necessary.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/StringToTransportProtocolConverter.java line 14 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
It depends on the values that we want to use in the configuration yaml.
I opted for the same wording as OpenTelemetry, which is
grpc,http/thrift,http/protobuf, andhttp/json.
Due to the slash, I have to implement a custom parse that parses these strings into their enum representation.To get rid of the
StringToTransportProtocolConverter, we would need to change from using/in the protocols to_, which is not in line with the naming of OTEL.
Add documentation to the class stating this.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java line 12 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I removed the unused
HTTP_JSON.
RegardingUNKNOWNandCOUNT, let's discuss this the weekly
As discussed, please remove :)
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java line 37 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
See comment above.
I use this so that we can use
/in the protocols to be in line with the naming of OTEL.
ok
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/OtlpMetricsExporterSettings.java line 35 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Good point.
Can we implement a generic Validator for Enum?
As far as I am aware, we can only implement a specific Validator for specific enums, e.g., for
TransportProtocolit would look like this:/** * Validate that an {@link TransportProtocol} is in the supported array of {@link #anyOf()}. * Will fail if a property is not in {@link #anyOf()}. */ @Target({METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE}) @Retention(RUNTIME) @Documented @Constraint(validatedBy = TransportProtocolSubsetValidator.class) public @interface TransportProtocolSubset { TransportProtocol[] anyOf(); String message() default "must be any of {anyOf}"; Class<?>[] groups() default {}; Class<? extends Payload>[] payload() default {}; }/** * Validator for {@link TransportProtocolSubset} */ public class TransportProtocolSubsetValidator implements ConstraintValidator<TransportProtocolSubset, Enum> { private TransportProtocol[] subset; @Override public void initialize(TransportProtocolSubset constraintAnnotation) { subset = constraintAnnotation.anyOf(); } @Override public boolean isValid(Enum value, ConstraintValidatorContext context) { return Arrays.asList(subset).contains(value); } }
Let's do this
There was a problem hiding this comment.
Reviewed 3 of 120 files at r5.
Reviewable status: 82 of 131 files reviewed, 66 unresolved discussions (waiting on @heiko-holz and @mariusoe)
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 6 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
-> create github issue for consistent interface naming
done: #1421
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 13 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As discussed, let's keep it consistent with the current code base for now.
I removed the IOpenTelemetry.noop()
inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 29 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As discussed, let's rename
isConfiguredtoisActiveand rethink whetherisStartedis necessary.
I renamed isConfigured to isActiveand removed the isEnabed/isStarted property.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/StringToTransportProtocolConverter.java line 14 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Add documentation to the class stating this.
I added a more detailed class documentation as well as an additional comment to configRepresentation.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/TransportProtocol.java line 12 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As discussed, please remove :)
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/InfluxExporterSettings.java line 34 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
See comment in
InfluxExporterService-> depending whether we support InfluxDB in the future, we should already add a deprecation warning or something like that.
In case we decide to deprecate InfluxExporterService in version 2.X, would you add the deprecation warning to the Settings or Serviceclass?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/OtlpMetricsExporterSettings.java line 35 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Let's do this
Actually, as discussed, it is not as easy as I thought.
If we want to have the validation in the Settings class using annotations, we need to discuss the following points:
- shall the validation also apply when the exporter is disabled?
- depending on the default value, this results in additional requirements such as conditional validation, see comments below
- what is the default value of
protocol?- currently, we use
nullas the default value so that the user has to specify it (which I think is fine in case the exporter is disabled). However, usingnullas the default value for disabled exporter would result in conditional validation, which needs to be implemented additionally - if we decide to use a supported value as default (e.g.,
grpc), then we need to set it in theexporter.yml. - if we decide to use
unsetas the default value, I need to add it to the list of supported protocols and implement an additional conditional validation that checks that the value is notunsetif the exporter is enabled
- currently, we use
Another option for the validation in the Settings class could be the @AdditionalValidations and @AdditionalValidation annotation, e.g.,
@AdditionalValidations
public class OtlpMetricsExporterSettings{
[....]
private static List<TransportProtocol> SUPPORTED_PROTOCOLS = Arrays.asList(TransportProtocol.GRPC, TransportProtocol.HTTP_PROTOBUF);
@AdditionalValidation
public void performValidations(ViolationBuilder vios) {
if (enabled.equals(ExporterEnabledState.ENABLED)) {
if (!SUPPORTED_PROTOCOLS.contains(protocol)) {
vios.atProperty("protocol")
.message("Unsupported value '{protocol}' for property 'protocol'. Supported values are {values}")
.parameter("protocol", protocol)
.parameter("values", Arrays.toString(SUPPORTED_PROTOCOLS.toArray()))
.buildAndPublish();
}
}
}
}I think we can implement the conditional validation in the performValidations method. But I honestly don't know whether it is advantageous having such a validation in the Settings class rather than in the Service class (which is technically not the same as validating the inputs).
If we decide to use validation, we need to consider that the configuration is rejected as soon as one validation fails (currently, the configuration is still accepted and only the ill-defined exporters are disabled)
Let's discuss this.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/JaegerExporterSettings.java line 43 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Since many exporters are using the
inspectit.service-nameattribute directly, we should consider to drop this attribute on individual exporter setting so it is consistent.
I agree.
I forgot to remove it from OtlpTraceExporterSettings and LoggingTraceExporterService (the property was not used and was also not in the documentation of these exporters).
Regarding the JaegerExporterSettings, I am not sure how the TraceExportersConfiguration in the EUM component needs to be changed, as I did not implement that class.
Should I move this property up to the EumServerConfiguration?
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/OtlpTraceExporterSettings.java line 21 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo this cannot happen that URL is set because until now, there are not OTLP settings. So
urlcan be removed.
You are right. The now deprecated version was never released in the feature/open-telemetry-migration branch.
I have removed the deprecated property.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/tracing/TracingSettings.java line 63 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As far as I can see, there is no benefit for using a seperate static field for the default value. Imo it makes the code much more complicated and harder to maintain.
Keep it simple and consinsten and move the default values directly to the actual field or/and add them to the default configuration file, so the configuration file can used as a sample for all default values of the configuration.
Thanks for pointing this out.
I was following the coding convention of OTEL (e.g., https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java).
If I am not mistaken, the code base mostly has the default values in the default configuration files, and does not set them in the code.
Thus, I removed the static fields and moved the value into the basics.yml of the default configuration.
However, the code base is not consistent. For example, the default value forTracingSettings#propagationFormat is also set in the code and in the default configuration.
What do you think, where should the default value be specified?
- directly in the field
- in the default configuration
- in both places
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml line 55 at r5 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about using
UNSETas default value?
I am not sure about using unset (using lower case to be consistent with the configuration; it will still be correctly parsed).
null clearly states that this value has not been set.
We also have been using nullfor other String properties before (e.g., endpoint, user, password etc.).
But I don't have a strong opinion with this.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/spring/BootstrapInitializerConfiguration.java line 60 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not needed
Thanks for spotting this. I removed the method parameter InspectitEnvironment environment.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/DynamicallyActivatableMetricsExporterService.java line 21 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Similar to the comment in the
DynamicallyActivatableTraceExporterServiceclass.We can refactor the method
registerMetricExporterService(DynamicallyActivatableMetricsExporterService service)of theOpenTelemetryControllerto the following:registerMetricExporterService(MetricReaderFactory factory, String name)then we don't need this class and can directly use the
DynamicallyActivatableServicewhich makes it simple in my oppinion.
I actually need to register something that offers a method that returns a new MetricReaderFactory.
When the SdkMeterProvider is shut down during the re-configuration of the OpenTelemetryControllerImpl, the previously registered MetricReaderFactory are also closed and cannot be re-used.
I therefore need to create a new MetricReaderFactory during OpenTelemetryControllerImpl#configureMeterProvider(InspectitConfig configuration).
As alternative to the DynamicallyActivatableMetricsExporterService class, we can also use a Callable<MetricReaderFactory as parameter of the registerMetricExporterService(Callable<MetricReaderFactory> newMetricReaderFactoryProvider, String serviceName.
What do you think?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java line 24 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Is this exporter still working? It is based on OpenCensus, right. It will stop working with the final migration from Bridge to OpenTelemetry - I guess..
Depending whether we provide an InfluxDB exporter for OpenTelemetry (which I would not recommend), we should discuss dropping support for direct InfluxDB metric exportation. As an alternative, the OTel collector can be used.
It it working with the OTEL-OC-Shim (although the InfluxExporterServiceIntTest still does not work reliably in a local development environment).
I also think that we do not need to provide an InfluxDB exporter as the OTEL Collector provides one (https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/influxdbexporter).
We can then just point to the OTEL Collector.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingMetricExporterService.java line 57 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
getName()
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingMetricExporterService.java line 59 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Would it not be sufficient to log:
log.error("Failed to register {} at the OpenTelemetry controller!", getName());
Yes, I changed this accordingly for all trace/metric exporter services
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/LoggingTraceExporterService.java line 51 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
See comments in the
LogginmetricExporterService
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/PrometheusExporterService.java line 35 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you replace this val with the actual class
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/ZipkinExporterService.java line 55 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please remove this inline comments
Done.
I also removed the inline comments in the doEnable method of the other trace/metric exporter services.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHook.java line 16 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Revert please.
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 156 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please remove this inline comment because the code itself is quite obvious :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 165 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not used. Just implement and keep the code we need. Otherwise we have to maintain dead code which is not good.
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 179 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Put this comment into the Javadoc
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 193 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Try to reduce the amount of inlince comments. Inline comments should usually be rather avoided to keep the code small and readable. If many inline comments are needed, it often indicates that there is something wrong with the code, e.g. that it is too complex or too large, or that parts of methods should perhaps be outsourced in order to keep a method small and simple (keyword: single responsiblity).
Thanks for pointing this out. I'll try to follow your advice :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 216 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please remove this inline comment because the code itself is quite obvious :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 220 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please remove this inline comment because the code itself is quite obvious :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 234 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please remove this inline comment because the code itself is quite obvious :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 242 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Is it correct that this can be set to
trueeven the controller is not started? Isenablednot more or less the same asconfigured?
As discussed, I removed enabled and changed configured to active
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 244 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please prevent an assignment and return value within a single line because it is quite hard to get what is happening here.
Furthermore, is it ok to return
truein case the controller has already been started? It could be confusing in case the method returnstruebut the controller has not been started at all because it is already running. How about an exception?How about using something like this? It quite easy to read and you can easily see what is happening:
if (configured) { throw new IllegalStateException("The controller is already running and cannot be started again."); } else { configured = configureOpenTelemetry(); return configured; }
I agree.
start() is currently only called once in the startup of the controller.
I adhered to your suggestion. Now, if the controller was already up and running and one calls start() again, the exception is thrown.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 248 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Flushes all pending...
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 266 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
This can/should never happen due to the method synchronization.
Thanks for pointing this out.
I mixed-and-matches code with the implementation of OTEL.
What do you think is better: synchronized keyword or AtomicBoolean?
I think the reason why OTEL opted for AtomicBoolean is to have the logging when something leads to multiple calls of the method.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 267 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Do we need some exception or
returncall here? Now, the shutdown will be execute even there are multiple shutdown calls.
You are right.
I think return would be appropriate - depending on how we resolve the above comment.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 318 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
the building of the tracer provider can be extracted into a separate method, the you don't need all these inline comments because the methods are much smaller
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 335 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 346 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo these comments are not needed.
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 349 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you use a more expressive log message
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 371 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 389 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Argument not needed
Thanks for spotting this!
I removed the parameter.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 397 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
not needed
Removed long start and the unused log statement.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 399 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Remove commented out logger code
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 401 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo comment like this should be remove because the code itself is quite obvious what's happening here :)
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 411 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
return builder.build();
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 422 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please remove Javadoc code in case the documentation is not filled out, like this parameter or the return value of this method.
Added Javadoc to complete the method's documentation.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 427 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
This can never be null.. At least I don't see any call where it is set to
null
Thanks for spotting this.
Removed the code block.
What do you advise in general: directly setting the empty map as the default value of the field, or using null as the default value and constructing the empty map here?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 468 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Such information should be in the Javadoc instead as an inline comment
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 497 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
This cannot be null.
Removed, see also comment for registerTraceExporterService.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 25 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
I would not use the builder pattern because there is only a single field to set.
I would recommend using the
AllArgsConstructorannotation and makinglockfinal:@AllArgsConstructor public class OpenTelemetryImpl implements OpenTelemetry { @NonNull private OpenTelemetrySdk openTelemetry; private final Object lock = new Object();Then instead of the following
OpenTelemetryImpl.builder().openTelemetry(openTelemetrySdk).build();it is just
new OpenTelemetryImpl(openTelemetrySdk);.
Regarding the builder pattern: I followed the implementation of the OpenTelemetrySdk which uses the builder pattern.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 41 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can be private - or can be remove in case you add
@NonNullto theopenTelemetryfield so it cannot be null at all
Yes, you are right.
I removed the method and annotated openTelemetry with @NotNull
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 83 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not used, thus, can be removed
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 97 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo its fine to remove these comments, the code is quite expressive. And if you are very strict, this comment is not quite correct either, because the provider is stopped if the method parameter is
true- which theoretically can be, if the settings haven't changed :P This is also one of the reasons why inline comments should rather be avoided, because you have to maintain not only the code but also the comments...
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 118 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
This lock would only make sense if we have multiple
OpenTelemetryImplinstances. Is that possible?
Actually, lock is not static.
I wanted to avoid that multiple calls to close or set result in unpredictable code.
Currently this should not be possible as set is only called from OpelTelemetryControllerImpl#configureOpenTelemetry, which is also synchronized.
Let's discuss the proper strategy for this (also whether I should make the method synchronized or use synchronized(lock)).
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 134 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
You can pass a method reference directly:
resultCode.whenComplete(latch::countDown);
Thanks!
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 138 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Use some nice log message.
Shall we really log the exception thrown by latch.await?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 149 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please remove this comment
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 151 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
return value is never needed and can be removed
Done.
inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md line 19 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
We use always an lower-case
iforinspectIT
Done.
inspectit-ocelot-documentation/docs/tracing/tracing.md line 32 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Move the "note" into a "warning block".
:::warning These properties take only effect once when the agent is starting. If you change these properties while the agent is running, they will not take effect until the agent retarted. :::Then it is rendered like this one: https://inspectit.github.io/inspectit-ocelot/docs/configuration/agent-command-configuration
Thanks.
I didn't know of :::warning and :::note. It doesn't render in IntelliJ.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/DynamicallyActivatableTraceExporterService.java line 12 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
I don't get the point why we need this class. The only time
getSpanExporteris used is during registration in theOpenTelemetryCollector#registerTraceExporterServicewhere an instance of this class is passed.We could also refactor that method from
registerTraceExporterService(DynamicallyActivatableTraceExporterService service)to
registerTraceExporterService(SpanExporter exporter, String name)then we don't need this class and can just use
DynamicallyActivatableService
Good point.
I removed this class and refactored the register and unregister method as advised.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/context/ContextUtil.java line 46 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not used.
Removed the method.
There was a problem hiding this comment.
Reviewable status: 82 of 131 files reviewed, 66 unresolved discussions (waiting on @heiko-holz and @mariusoe)
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/logging/Log4J2TraceIdAutoInjectorTest.java line 36 at r6 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Use the
LOGGERfor logging
I removed the unnecessary debug log statement. Using LOGGER breaks the tests (as it asserts on the Log42JoggingRecorder.loggingEvents).
General question: when do we use LoggerFactory.getLogger(Class.class), Logger.getLogger(Class.class.getName()), and when LogManager.getLogger(Class.class) (from org.apache.log4j) ?
Let's create a github issue that investigates the use of different logging frameworks / access to a logger to see if we inconsistently use different libraries / calls to retrieve a logger. As a result of the ticket, we could have a table detailing in which content which library/call is used.
…7-integrate-jaeger-proto-and-otlp-exporters
There was a problem hiding this comment.
Reviewed 25 of 38 files at r7, 2 of 3 files at r8.
Dismissed @heiko-holz from a discussion.
Reviewable status: 99 of 131 files reviewed, 9 unresolved discussions (waiting on @heiko-holz and @mariusoe)
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/InfluxExporterSettings.java line 34 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
In case we decide to deprecate
InfluxExporterServicein version2.X, would you add the deprecation warning to theSettingsorServiceclass?
We keep the exporter for now.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/trace/JaegerExporterSettings.java line 43 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I agree.
I forgot to remove it from
OtlpTraceExporterSettingsandLoggingTraceExporterService(the property was not used and was also not in the documentation of these exporters).Regarding the
JaegerExporterSettings, I am not sure how theTraceExportersConfigurationin the EUM component needs to be changed, as I did not implement that class.
Should I move this property up to theEumServerConfiguration?
You should be able to remove this because the EUM server is not using a service name. The service name of the EUM spans are already specified by the EUM agent, thus we don't need it there.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/tracing/TracingSettings.java line 63 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Thanks for pointing this out.
I was following the coding convention of OTEL (e.g., https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java).
If I am not mistaken, the code base mostly has the default values in the default configuration files, and does not set them in the code.
Thus, I removed the static fields and moved the value into thebasics.ymlof the default configuration.However, the code base is not consistent. For example, the default value for
TracingSettings#propagationFormatis also set in the code and in the default configuration.What do you think, where should the default value be specified?
- directly in the field
- in the default configuration
- in both places
Let's keep it as you did in the latest version. We need to discuss this with the team.
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml line 55 at r5 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I am not sure about using
unset(using lower case to be consistent with the configuration; it will still be correctly parsed).
nullclearly states that this value has not been set.We also have been using
nullfor other String properties before (e.g.,endpoint,user,passwordetc.).But I don't have a strong opinion with this.
let's use null
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/DynamicallyActivatableMetricsExporterService.java line 21 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I actually need to register something that offers a method that returns a new
MetricReaderFactory.When the
SdkMeterProvideris shut down during the re-configuration of theOpenTelemetryControllerImpl, the previously registeredMetricReaderFactoryare also closed and cannot be re-used.
I therefore need to create a newMetricReaderFactoryduringOpenTelemetryControllerImpl#configureMeterProvider(InspectitConfig configuration).As alternative to the
DynamicallyActivatableMetricsExporterServiceclass, we can also use aCallable<MetricReaderFactoryas parameter of theregisterMetricExporterService(Callable<MetricReaderFactory> newMetricReaderFactoryProvider, String serviceName.What do you think?
Ok, got it. But then let's not name it get.., as this may not be understood correctly ( as it was the case with me :) ).
What about createMetricReaderFactory? This clearly states that it creates a new factory.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java line 24 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
It it working with the OTEL-OC-Shim (although the
InfluxExporterServiceIntTeststill does not work reliably in a local development environment).I also think that we do not need to provide an InfluxDB exporter as the OTEL Collector provides one (https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/influxdbexporter).
We can then just point to the OTEL Collector.
Let's keep it for now, so the migration is easier
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java line 50 at r8 (raw file):
if ((null == jaeger.getProtocol() || jaeger.getProtocol() .equals(TransportProtocol.UNSET)) && (StringUtils.hasText(jaeger.getUrl()) || StringUtils.hasText(jaeger.getGrpc()))) { jaeger.setProtocol(StringUtils.hasText(jaeger.getUrl()) ? TransportProtocol.HTTP_THRIFT : TransportProtocol.GRPC);
Imo, we should consider the configuraion element as immutable and use local variables instead. Changing it might not be the best way (feels not good) because I assume it modifies the configuration object itself.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java line 51 at r8 (raw file):
.equals(TransportProtocol.UNSET)) && (StringUtils.hasText(jaeger.getUrl()) || StringUtils.hasText(jaeger.getGrpc()))) { jaeger.setProtocol(StringUtils.hasText(jaeger.getUrl()) ? TransportProtocol.HTTP_THRIFT : TransportProtocol.GRPC); log.warn("The property 'protocol' was not set. Based on the set property '{}' we assume the protocol '{}'. This fallback will be removed in future releases. Please make sure to use the property 'protocol' in future.", StringUtils.hasText(jaeger.getUrl()) ? "url" : "grpc", StringUtils.hasText(jaeger.getUrl()) ? TransportProtocol.HTTP_THRIFT.getConfigRepresentation() : TransportProtocol.GRPC.getConfigRepresentation());
Can you move these inline if into local variables? It makes the code quite hard to read when all code is in a single line/statement.
Code quote:
StringUtils.hasText(jaeger.getUrl()) ? "url" : "grpc", StringUtils.hasText(jaeger.getUrl()) ? TransportProtocol.HTTP_THRIFT.getConfigRepresentation() : TransportProtocol.GRPC.getConfigRepresentation()inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java line 79 at r8 (raw file):
try { JaegerExporterSettings settings = configuration.getExporters().getTracing().getJaeger(); String endpoint = StringUtils.hasText(settings.getEndpoint()) ? settings.getEndpoint() : StringUtils.hasText(settings.getUrl()) ? settings.getUrl() : settings.getGrpc();
Please break it up in multiple calls. Easier to read and it is less error-prone :)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 266 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Thanks for pointing this out.
I mixed-and-matches code with the implementation of OTEL.What do you think is better:
synchronizedkeyword orAtomicBoolean?I think the reason why OTEL opted for
AtomicBooleanis to have the logging when something leads to multiple calls of the method.
Hmm... I think it depends. Yes, using the boolean you can print some kind of warning but on the other hand, you don't need additional variables (the boolean) when using synchronized.
I would vote for synchronization, because it is less code and the only benefit (for the boolean) is the log statement as you mentioned.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 427 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Thanks for spotting this.
Removed the code block.What do you advise in general: directly setting the empty map as the default value of the field, or using
nullas the default value and constructing the empty map here?
I would set it directly using a default value because I see no benefit not initilizing it directly in the class field.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryImpl.java line 25 at r6 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Regarding the builder pattern: I followed the implementation of the
OpenTelemetrySdkwhich uses the builder pattern.
I think builder pattern is nice when many arguments are used, but in our case it looks a bit too much :) You also get a method for setting the lock here, which is not needed. Let's use it without a builder.
…aeger-proto-and-otlp-exporters' into features/feat-1297-integrate-jaeger-proto-and-otlp-exporters # Conflicts: # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/utils/HighPrecisionTimerTest.java
Codecov Report
@@ Coverage Diff @@
## feature/opentelemetry-migration #1375 +/- ##
=====================================================================
- Coverage 81.29% 79.16% -2.13%
- Complexity 2111 2219 +108
=====================================================================
Files 215 231 +16
Lines 6691 7179 +488
Branches 785 848 +63
=====================================================================
+ Hits 5439 5683 +244
- Misses 948 1149 +201
- Partials 304 347 +43
|
heiko-holz
deleted the
features/feat-1297-integrate-jaeger-proto-and-otlp-exporters
branch
This change is