Move Dynatrace metrics exporter for API v1 to a separate package

[pirgeo]

This is a PR in preparation for the Dynatrace API v2 exporter (#2295). It moves the current v1 implementation into the v1 package while keeping the interface unchanged, making this a compatible change.

[pivotal-issuemaster]

@pirgeo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@pirgeo Thank you for signing the Contributor License Agreement!

[jonatan-ivanov]

Since this is an enum now, this check is not necessary, enums are type-safe and since there is only one value: V1 this can't be anything else than V1 unless somebody overrides it but in that case they will overwrite validate too.

What I would do here is the following:

  @Override
  default Validated<?> validate() {
      return checkAll(this,
              c -> StepRegistryConfig.validate(c),
              checkRequired("apiToken", DynatraceConfig::apiToken),
              checkRequired("uri", DynatraceConfig::uri),
              checkRequired("deviceId", DynatraceConfig::deviceId),
              check("technologyType", DynatraceConfig::technologyType).andThen(Validated::nonBlank),
              check("apiVersion", DynatraceConfig::apiVersion)
      );
  }

Because of the version is an enum, this will try to convert it (by calling apiVersion) and if that call won't be successful (e.g.: invalid value), it will make the result invalid. Please see my comment on the test class of this.

[pirgeo]

This is much easier, and I adopted it for the v1 exporter. However, for the v2 exporter, we will only require the API version and checking these four required parameters with a .andThen(v -> v.invalidateWhen(<check-version-here>) construct for v1 seems a bit hard to read. Is there another way to make this more concise?

[jonatan-ivanov]

InfluxConfix does something similar though, in this case, I would create a validateV1 and a validateV2 method (the current validate becomes validateV1). With them you can do:

@Override
default Validated<?> validate() {
    return apiVersion() == V1 ? validateV1() : validateV2();
}

[pirgeo]

I split it out to a separate function, and also removed the invalidVersion test, as the test data is invalid and can never happen in a non-test environment. If the spring boot app is started with an application property of v-INVALID, for example, it will just crash on startup and provide all valid values (which is just V1) in this case.
Another way to get around this would be to explicitly validate the apiVersion before accessing it, something like the following:

@Override
default Validated<?> validate() {
    Validated<DynatraceApiVersion> apiVersionValidation = checkRequired("apiVersion", DynatraceConfig::apiVersion).apply(this);
    if (apiVersionValidation.isInvalid()) {
        return apiVersionValidation;
    }
    return apiVersion() == V1 ? validateV1() : validateV2();
}

which seems a bit unnecessary if this case never actually appears during use.

[pirgeo]

I just realized that that is only the case when using micrometer with Spring Boot. I think the solution I posted above (with the additional step of validating the apiVersion) should work, if i just access the apiVersion() method without validation first I recieve a ValidationException. I added the additional validation step.

[pirgeo]

Hi @jonatan-ivanov, I think I addressed all the comments above. Please let me know if there's anything missing so we can move on with this. Thanks!

[jonatan-ivanov]

@pirgeo Sorry for not getting back to you earlier. I merged your PR but not yet into master.
We are only merging smaller fixes to master between the RC1 and the GA version of 1.7.0. We can branch off 1.7.x but that means one more branch to maintain for us (we will do it after the release).

I'm going to try/fix a few things (validation*) on the move-dynatrace-registry-v1 branch but in the meantime, you can handle this branch as master and build the v2 feature on top of it.

validation*: I totally forgot that Java only supports private methods in interfaces from Java 9. I'm not sure we should have a public validateV1 method on the config interface.

[jonatan-ivanov]

@pirgeo please let me know what do you think: 4f53492

[jonatan-ivanov]

@pirgeo I also broke the circular dependency between the registry and the exporter: 1115208

[pirgeo]

@jonatan-ivanov Thanks a lot, these changes look good to me!