Closes #1555: [Feature] - fix StackTraceSampler / AutoTracing #1558

heiko-holz · 2022-12-23T18:46:36Z

This change is

Dimi-Ma

Reviewed 81 of 81 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @heiko-holz)

a discussion (no related file):
There were a few inconsistencies regarding code formatting, maybe we should discuss this in a Meeting.

inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/logging/Log4J2TraceIdAutoInjectorTest.java line 58 at r1 (raw file):

        Span span = tracer.spanBuilder("test").startSpan();
        try (Scope scope = span.makeCurrent()) {
            traceId = Span.current().getSpanContext().getTraceId();

in LogCorelationTest.java this part was extracted into their own function so to keep everything the same we should do this here too or move the previous implementation into a higher order to be able to use this function everywhere its needed and don't have redundancy.

inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/logging/Log4JTraceIdAutoInjectorTest.java line 36 at r1 (raw file):

        try (Scope scope = tracer.spanBuilder("test").startSpan().makeCurrent()) {
            traceId = Span.current().getSpanContext().getTraceId();

Same here for the extraction into their own function.

inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/opentelemetry/IOpenTelemetryController.java line 61 at r1 (raw file):

    /**
     * Registers a new {@link rocks.inspectit.ocelot.core.service.DynamicallyActivatableService trace exporter service} that is used to export {@link io.opentelemetry.sdk.trace.data.SpanData} for sampled {@link io.opentelemetry.api.trace.Span}s
     * This method should ONLY be used in tests of the {@code agent} package.

Is it a good idea to implement a test only function here?

inspectit-ocelot-core/src/main/java/io/opentelemetry/sdk/trace/OcelotAnchoredClockUtils.java line 87 at r1 (raw file):

            throw new RuntimeException(e);
        }

This space is not needed.

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolver.java line 55 at r1 (raw file):

        boolean traceEntryHook = Stream.of(matchedRules.stream()
                .map(InstrumentationRule::getPreEntryActions)
                .flatMap(Collection::stream), matchedRules.stream()

I think it would be better to format this one with the commas instead of the dots to make it more readable for each value. e.g.

Code snippet:

        boolean traceEntryHook = Stream.of(
            matchedRules.stream().map(InstrumentationRule::getPreEntryActions).flatMap(Collection::stream), 
            matchedRules.stream().map(InstrumentationRule::getEntryActions).flatMap(Collection::stream), 
            matchedRules.stream().map(InstrumentationRule::getPostEntryActions).flatMap(Collection::stream)).flatMap(s -> s).anyMatch(ActionCallConfig::isActionTracing);
            ...

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/context/InspectitContextImpl.java line 3 at r1 (raw file):

package rocks.inspectit.ocelot.core.instrumentation.context;

import io.opencensus.tags.*;

Is this something that can't be done with open telemetry?

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java line 176 at r1 (raw file):

        String sampleProbability = tracing.getSampleProbability();
        if (!StringUtils.isBlank(sampleProbability)) {

this empty line can be removed.

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/trace/samplers/HybridParentTraceIdRatioBasedSampler.java line 52 at r1 (raw file):

        // if any parent link has been sampled, keep the sampling decision.
        if (isAnyParentLinkSampled) {

Could we not put this if into the previous if statement with an OR since they are both returning the same?

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/utils/OpenCensusShimUtils.java line 90 at r1 (raw file):

    }

    // TODO: über Reflection auf die Copy-Helper-Methode zuzugreifen, anstatt diese zu kopieren

Is this TODO supposed to be finished in this PR? If not it might be a better idea to create a separate Ticket for it before it gets forgotten.

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/WriteSpanAttributesTest.java line 30 at r1 (raw file):

    IObfuscatory obfuscatory;

    @Spy

Do we still need to use @Spyhere? If yes, a comment explaining why would be helpful 🙂

Dimi-Ma

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @heiko-holz)

a discussion (no related file):