Closes #1248 - Refactor enabled field for exporters

[aaronweissler]

This change is 

[codecov]

Codecov Report

Merging #1303 (1f9c55a) into master (d37a7dc) will decrease coverage by 1.88%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1303      +/-   ##
============================================
- Coverage     83.00%   81.12%   -1.88%     
- Complexity     1736     2066     +330     
============================================
  Files           174      209      +35     
  Lines          5366     6584    +1218     
  Branches        650      783     +133     
============================================
+ Hits           4454     5341     +887     
- Misses          675      945     +270     
- Partials        237      298      +61     

Impacted Files	Coverage Δ
...spectit/ocelot/core/utils/WeakMethodReference.java	0.00% <0.00%> (-77.27%)	⬇️
...strap/correlation/noop/NoopLogTraceCorrelator.java	28.57% <0.00%> (-42.86%)	⬇️
...celot/core/exporter/PrometheusExporterService.java	81.48% <0.00%> (-13.97%)	⬇️
...ial/ScheduledExecutorContextPropagationSensor.java	82.05% <0.00%> (-12.39%)	⬇️
...pectit/ocelot/config/loaders/ConfigFileLoader.java	82.61% <0.00%> (-8.70%)	⬇️
...lot/bootstrap/context/noop/NoopContextManager.java	10.00% <0.00%> (-6.67%)	⬇️
...core/instrumentation/InstrumentationTriggerer.java	85.19% <0.00%> (-5.63%)	⬇️
...rocks/inspectit/ocelot/config/utils/CaseUtils.java	90.48% <0.00%> (-4.98%)	⬇️
...t/ocelot/core/instrumentation/hook/MethodHook.java	95.35% <0.00%> (-4.65%)	⬇️
...del/metrics/definition/ViewDefinitionSettings.java	45.45% <0.00%> (-4.55%)	⬇️
... and 110 more

[aaronweissler]

There is one main question for me still left with this, whether and if so how the enabled flag should actually be refactored? Because I noticed it has actually always already done what was asked for in the issue's comment: "The exporter will only be started when this flag is true AND when all necessary parameters have been configured."

To me personally the name enabled actually is a bit more fitting than enabledIfConfigured, since as we talked about not every exporter needs to be configured and I'd find that name confusing for them while to me enabled for the ones needing additional configuration seems fine with the newly added warnings.

But of course I'm looking forward to any differing opinions too.

[aaronweissler]

Reviewable status: 0 of 16 files reviewed, 10 unresolved discussions

---

inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml, line 32 at r1 (raw file):

        # the export interval of the metrics
        export-interval: ${inspectit.metrics.frequency}

I added these simply for the influx part to be more in line with the others

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java, line 46 at r1 (raw file):

    @Override
    protected boolean checkEnabledForConfig(InspectitConfig conf) {
        InfluxExporterSettings influx = conf.getExporters().getMetrics().getInflux();

I removed two of the statements in the if-statement because as far as I understand neither of database and retentionPolicy can be empty anyways, since they are both annotated with NotBlank in InfluxExporterSettings.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java, line 47 at r1 (raw file):

    protected boolean checkEnabledForConfig(InspectitConfig conf) {
        InfluxExporterSettings influx = conf.getExporters().getMetrics().getInflux();
        if(conf.getMetrics().isEnabled() && influx.isEnabled()){

!isEmpty was replaced with hasText because isEmpty has been deprecated.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java, line 5 at r1 (raw file):

import io.opencensus.exporter.trace.jaeger.JaegerExporterConfiguration;
import io.opencensus.exporter.trace.jaeger.JaegerTraceExporter;
import lombok.extern.slf4j.Slf4j;

Some of the ExporterServices were using StringUtils from apache.commons.lang3 while others were using a more recent version from spring, so I updated the former to the latter as well.

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/InvalidConfigurationUpdateTest.java, line 21 at r1 (raw file):

            mp.setProperty("inspectit.service-name", "ConfA");
        });
        Assertions.assertThat(env.getCurrentConfig().getServiceName()).isEqualTo("ConfA");

The newly created warnings would make this test fail, since none of the exporters get their necessary values set in this test but are enabled by default.

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentMetricsExporterServiceIntTest.java, line 59 at r1 (raw file):

    //@Test This test is currently deactivated, since the current implementation of the trace exporter tries to connect to a google service before starting
    // and the request runs into a timeout.
    public void testGrpcRequest() {

I moved these properties into the test case, since they are only needed there as of now, and the new test would not work with them in TestPropertySource, because the OpenCensus Metrics Exporter can not be restarted when its properties change, so the warning would never be triggered.

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/PrometheusExporterServiceIntTest.java, line 19 at r1 (raw file):

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.catchThrowable;

Added because the tests expect the Prometheus Exporter to be activated.

---

inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md, line 5 at r1 (raw file):

title: Breaking Changes
---

I am not sure whether this text actually expresses everything properly, but I noticed Marius mentioning it was a breaking change, so I thought I'd try to add it already, so it won't be forgotten.

---

inspectit-ocelot-documentation/docs/metrics/metric-exporters.md, line 41 at r1 (raw file):

Metrics can be additionally exported to the [OpenCensus Agent](https://opencensus.io/service/components/agent/).
When enabled, all metrics are sent via gRCP to the OpenCensus Agent. By default, the exporter is enabled, but the agent address that is needed for the exporter to actually start is set to `null`.

I guess to most people it would be obvious but I felt like it was a good idea to express it a bit more clearly that the exporter won't start without setting the url.

---

inspectit-ocelot-documentation/docs/tracing/trace-exporters.md, line 16 at r1 (raw file):

## Zipkin Exporter

The Zipkin exporter exports Traces in Zipkin v2 format to a Zipkin server or other compatible servers.

Zipkin and Jaeger exporters' documentation was not structured like others with an easily overviewable table, I felt like it was a good idea to change that too when already updating some of the docs regarding exporters.

[aaronweissler]

Reviewable status: 0 of 24 files reviewed, 9 unresolved discussions

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/InvalidConfigurationUpdateTest.java, line 21 at r1 (raw file):

Previously, aaronweissler wrote…

The newly created warnings would make this test fail, since none of the exporters get their necessary values set in this test but are enabled by default.

Could be reverted again due to the now default IF_CONFIGURED not triggering any warnings.

[aaronweissler]

Reviewable status: 0 of 32 files reviewed, 10 unresolved discussions

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java, line 54 at r3 (raw file):

            log.info("Starting Jaeger Exporter on grpc '{}'", jaegerExporterSettings.getGrpc());
        }

I removed those lines, because they are unreachable due to the Conditional annotations.

[heiko-holz]

I think we have a nice solution now with using enum instead of a single Boolean.

Thanks for your great refactoring job!

Reviewed 4 of 23 files at r2, 27 of 28 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r7, all commit messages.
Dismissed @aaronweissler from 9 discussions.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @aaronweissler)

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/InfluxExporterService.java, line 49 at r5 (raw file):

        return influx.isEnabled()
                && !StringUtils.isEmpty(influx.getUrl())
                && !StringUtils.isEmpty(influx.getDatabase())

same discussion with getDatabase() as for the inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java, line 46 at r7 (raw file):

    @ConditionalOnProperty({"inspectit-eum-server.exporters.tracing.jaeger.enabled", "inspectit-eum-server.exporters.tracing.jaeger.grpc"})
    @ConditionalOnExpression("NOT new String('${inspectit-eum-server.exporters.tracing.jaeger.enabled}').equals('DISABLED') AND new String('${inspectit-eum-server.exporters.tracing.jaeger.grpc}').length() > 0")
    public SpanExporter jaegerSpanExporter() {

Would it make sense to have a test class for TraceExportersConfiguration.java and test the conditions for public SpanExporter jaegerSpanExporter()?

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/BooleanToExporterEnabledStateConverter.java, line 9 at r5 (raw file):

@Slf4j
@Deprecated
public class BooleanToExporterEnabledStateConverter implements Converter<Boolean, ExporterEnabledState> {

Could you add a short comment why this class was originally used and is now deprecated?

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/PrometheusExporterSettings.java, line 32 at r4 (raw file):

     * The port on which the /metrics endpoint of prometheus will be started.
     */
    @Min(1)

Why did you change this to @Min(1)?

---

inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/exporters.yml, line 32 at r1 (raw file):

Previously, aaronweissler wrote…

I added these simply for the influx part to be more in line with the others

I agree, consistency is good.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java, line 46 at r1 (raw file):

Previously, aaronweissler wrote…

I removed two of the statements in the if-statement because as far as I understand neither of database and retentionPolicy can be empty anyways, since they are both annotated with @notblank in InfluxExporterSettings.

does this mean that the deserialization of the configuration will throw exceptions if these fields are not set?
We can talk in tomorrow's standup briefly whether we want to explicitly log warnings for all fields that should be set but are not set if ENABLED is set.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java, line 5 at r1 (raw file):

Previously, aaronweissler wrote…

Some of the ExporterServices were using StringUtils from apache.commons.lang3 while others were using a more recent version from spring, so I updated the former to the latter as well.

Just for understanding: we use org.springframework.util.StringUtils in favor of org.apache.commons.lang3.StringUtils because the former is more recent?

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterServiceIntTest.java, line 75 at r4 (raw file):

        });
        assertLogsOfLevelOrGreater(Level.WARN);
        assertLogCount("Jaeger Exporter is enabled but no url set.", 1);

see comment for inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentMetricsExporterServiceIntTest.java, line 142 at r5 (raw file):

        });
        assertLogsOfLevelOrGreater(Level.WARN);
        assertLogCount("OpenCensus Metrics Exporter is enabled but no address set.", 1);

see comment for inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java, line 218 at r4 (raw file):

        });
        assertLogsOfLevelOrGreater(Level.WARN);
        assertLogCount("OpenCensus Tracing Exporter is enabled but no address set.", 1);

I thinkt it would be better to just count the number of warnings. If we change the warning message, we will always have to copy&paste it to the test cases as well.

---

inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md, line 10 at r4 (raw file):

### New definition of exporters' enabled property

Instead of a Boolean, the enabled field of exporters is now an enum with the values `DISABLED`, `ENABLED` and `IF_CONFIGURED` to express the way this property behaves more clearly.

rather flag or property than field?

---

inspectit-ocelot-documentation/docs/metrics/metric-exporters.md, line 41 at r1 (raw file):

Previously, aaronweissler wrote…

I guess to most people it would be obvious but I felt like it was a good idea to express it a bit more clearly that the exporter won't start without setting the url.

Very nice, thanks!

---

inspectit-ocelot-documentation/docs/metrics/metric-exporters.md, line 73 at r5 (raw file):
does it log an error or a warning?

I would also change

with IF_CONFIGURED

to

if set to IF_CONFIGURED

Another note: shall we format ENABLED, DISABLED, and IF_CONFIGURED to code formatting?

This comment also applies to the other metrics/trace exporter configuration documentations

---

inspectit-ocelot-documentation/docs/tracing/trace-exporters.md, line 16 at r1 (raw file):

Previously, aaronweissler wrote…

Zipkin and Jaeger exporters' documentation was not structured like others with an easily overviewable table, I felt like it was a good idea to change that too when already updating some of the docs regarding exporters.

Thanks, that's really nice!

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/ExporterEnabledState.java, line 10 at r4 (raw file):

 */
public enum ExporterEnabledState {
    ENABLED, DISABLED, IF_CONFIGURED

do we rather want to rename the enum to ExporterState and then IF_CONFIGURED to ENABLED_IF_CONFIGURED?

Let's discuss in tomorrow's standup.

[aaronweissler]

Reviewable status: 12 of 35 files reviewed, 12 unresolved discussions (waiting on @aaronweissler, @heiko-holz, and @notblank)

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/InfluxExporterService.java, line 49 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

same discussion with getDatabase() as for the inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java

Done.

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/conversion/BooleanToExporterEnabledStateConverter.java, line 9 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Could you add a short comment why this class was originally used and is now deprecated?

Done.

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/metrics/PrometheusExporterSettings.java, line 32 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Why did you change this to @Min(1)?

As discussed, in the EUM-Server's PrometheusExporterService there was a now removed if-statement whether the port-number was unequal 0, and after talking with Marius, I removed that while instead enforcing it with the already existing annotations here in the settings.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/InfluxExporterService.java, line 46 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

does this mean that the deserialization of the configuration will throw exceptions if these fields are not set?
We can talk in tomorrow's standup briefly whether we want to explicitly log warnings for all fields that should be set but are not set if ENABLED is set.

Yes, as discussed, due to the notblank annotation a (not very expressive) exception will be thrown if the fields are not set or set to an empty string, before this if-statement is ever reached.

---

inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md, line 10 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

rather flag or property than field?

Done.
Yeah, it probably should be property, since that is what it's called elsewhere in the docs.

---

inspectit-ocelot-documentation/docs/metrics/metric-exporters.md, line 73 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

does it log an error or a warning?

I would also change

with IF_CONFIGURED

to

if set to IF_CONFIGURED

Another note: shall we format ENABLED, DISABLED, and IF_CONFIGURED to code formatting?

This comment also applies to the other metrics/trace exporter configuration documentations

Done.
It does log a warning, I forgot to also update the documentation when making that change after talking about it with Marius.
And I agree, they should probably be code formatted in the Description column too :)

---

inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/exporters/ExporterEnabledState.java, line 10 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

do we rather want to rename the enum to ExporterState and then IF_CONFIGURED to ENABLED_IF_CONFIGURED?

Let's discuss in tomorrow's standup.

Done.
As discussed in the standup, we'll keep the naming as is.

[aaronweissler]

Reviewable status: 12 of 36 files reviewed, 12 unresolved discussions (waiting on @aaronweissler, @heiko-holz, and @notblank)

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java, line 46 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Would it make sense to have a test class for TraceExportersConfiguration.java and test the conditions for public SpanExporter jaegerSpanExporter()?

I agree, it would be a good idea.

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterServiceIntTest.java, line 75 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

see comment for inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java

Done.

Code quote (from components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/configuration/TraceExportersConfiguration.java):

StringUtils

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentMetricsExporterServiceIntTest.java, line 142 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

see comment for inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java

Done.

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java, line 218 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

I thinkt it would be better to just count the number of warnings. If we change the warning message, we will always have to copy&paste it to the test cases as well.

Done.

[heiko-holz]

Reviewed 11 of 19 files at r8, 2 of 5 files at r9, 5 of 5 files at r11, 6 of 6 files at r12.
Reviewable status: 31 of 36 files reviewed, 1 unresolved discussion (waiting on @aaronweissler and @heiko-holz)

[heiko-holz]

Reviewed 5 of 5 files at r13, 3 of 3 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aaronweissler)

[heiko-holz]

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aaronweissler)

[aaronweissler]

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

---

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/EumExportersSettings.java, line 16 at r16 (raw file):

@EqualsAndHashCode(callSuper = true)
@Validated
public class EumExportersSettings extends ExportersSettings {

We decided to remove the inheritance and instead insert the metrics field directly here, because both the superclass and this class had a field called "tracing" leading to issues with Spring loading properties.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java, line 5 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Just for understanding: we use org.springframework.util.StringUtils in favor of org.apache.commons.lang3.StringUtils because the former is more recent?

Yes. As discussed, we also will decide on unifying the usage at different places in the project in a new issue.

[heiko-holz]

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

[heiko-holz]

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