More straight forward and uniform optionals for Metadata

[jbee]

This is a suggestion to make Metadata more predictable when it comes to difference between values being null (not present) and set to their defaults explicitly.

Metadata methods returning plain String or MetricType never return null,

• display name defaults to name,
• unit defaults to MetricUnits.NONE
• description defaults to ""
• type defaults to MetricType.INVALID

This default is always applied in last possible moment when getter reads the field value. Until then any value including null may be passed to builder with-methods. null simple makes values not present and triggers the defaults unless the value is accessed via one of the Optional-returning methods which give access to the underlying value in the sense that they allow to distinguish between a value that is not set (and returns its default) and a value that is set to any value that isn't the default.

The builder sets its field to null if the default value is passed to the with method so that any default means the field in null and will fallback to its default when accessed via plain getter and that any default will be treated as not present.

Reason this changes are suggested is that current state of Metadata and the builder is very confusing and hard to predict and different Metadata objects (with regards to their is present behaviour) can easily created by accident. Something that should be a simple and straight forward data record has a complexity to it that has no real benefit. Making sure plain methods never return null can be done without this complexity as this PR wants to show.

I also avoided use of default methods but naturally the fallback logic as demanded by the javadoc could be implemented as such making all plain methods default methods that use the -Optional methods as done by the DefaultMetadata implementation.

[eclipse-microprofile-bot]

Can one of the admins verify this patch?

[jmartisk]

ok to test

[jbee]

Updated the copyright of the API sources.

[Channyboy]

@donbourne any concerns?

[donbourne]

code changes look good.

While not introduced in this PR, I think it might be broken that we let "type" be optional. Is there any value to ever creating a Metadata instance that has type == MetricType.INVALID? Should we make type be required (like we do with name) in MetadataBuilder?

[jmartisk]

Type is required if you're passing the metadata to the register method where it's not clear from the method name what metric type you want. If people want to construct a metadata to be passed to metricRegistry.counter(metadata), there's no reason to force them to include the type in the metadata, and the implementation is supposed to "sanitize" such metadata automatically, as specified in https://github.com/eclipse/microprofile-metrics/pull/544/files

[donbourne]

Hmm. I think we're trading the certainty that your Metadata is valid against the convenience of not having to specify the type in the Metadata depending on the usage of the Metadata object. I guess the call to register is going to be rare compared to calls to counter and other methods that implicitly know the type -- so that tradeoff seems ok.

I've noticed other APIs (micrometer) actually put the Builder on the metric (eg. Counter), which might make this less of an issue.

[Channyboy]

@jbee there seems to be some merge conflicts before we can merge.

[jbee]

Rebased it, now the ECA check fails which I assume has to do with an eclipse admin merging my accounts. I did sign ECA again but that does not seem to help.

[jmartisk]

You have a syntax error in DefaultMetadata.java

[jmartisk]

Not sure why the ECA check is failing, perhaps it will fix itself after re-pushing, your account is showing as having signed it :confused:

[chrisguindon]

Not sure why the ECA check is failing, perhaps it will fix itself after re-pushing, your account is showing as having signed it 😕

I found 2 accounts in our database with the same email address. One of the account did not have an ECA on file.

This should be fixed now!

[jbee]

Thanks @chrisguindon . I'd assume that this was an unintended result of merging two of my eclipse account for different emails to one account. Check now passes.