Remove validations for noop instrument names and units

[breedx-splk]

We were chatting about API behavior and it was discovered that some metrics APIs do validation. We think that this is probably not what the spec intended, but also creates some log chatter and computational overhead in what should really be a true no-op implementation.

The instrument naming spec does, oddly enough, impose naming restrictions, but these are implemented in the sdk. The api does not seem like place for this behavior.

The metric api specification around units says that even the SDK isn't supposed to validate....so it makes little sense for the API to also be doing unit validations.

I think that it is unlikely that users are relying on this warning to be logged, so I believe that this change should be safe.

[breedx-splk]

@MrAlias @tsloughter FYI

[jack-berg]

The instrument naming spec does, oddly enough, impose naming restrictions, but these are implemented in the sdk. The api does not seem like place for this behavior.

FYI, here's where that was originally added. Essentially, we did the thing we deemed most correct given the ambiguity in the specification about how API level argument restrictions should actually be enforced.

The metric api specification around units says that even the SDK isn't supposed to validate....so it makes little sense for the API to also be doing unit validations.

I think the spec is contradictory on this point. It needs to resolve the "the SDK is not expected to validate the unit of measurement" phrasing with the "It can have a maximum length of 63 characters". Not sure how these can coexist. I brought up this contradiction in this issue.

I'd want to hold off on merging this until the spec provides more clarity. It could end up being the case that noop API is supposed to do this validation.

[codecov]

Codecov Report

Patch coverage: 94.59% and project coverage change: +0.02 🎉

Comparison is base (e185416) 91.05% compared to head (3d6a84a) 91.08%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5146      +/-   ##
============================================
+ Coverage     91.05%   91.08%   +0.02%     
- Complexity     4880     4883       +3     
============================================
  Files           549      550       +1     
  Lines         14428    14420       -8     
  Branches       1374     1376       +2     
============================================
- Hits          13138    13134       -4     
+ Misses          893      890       -3     
+ Partials        397      396       -1     

Impacted Files	Coverage Δ
...ava/io/opentelemetry/api/metrics/DefaultMeter.java	88.42% <ø> (-1.30%)	⬇️
.../sdk/metrics/internal/InstrumentNameValidator.java	88.88% <88.88%> (ø)
...lemetry/sdk/metrics/AbstractInstrumentBuilder.java	97.91% <90.90%> (-2.09%)	⬇️
...ntelemetry/api/baggage/ImmutableEntryMetadata.java	100.00% <100.00%> (ø)
.../io/opentelemetry/api/internal/ApiUsageLogger.java	100.00% <100.00%> (ø)
...java/io/opentelemetry/api/trace/DefaultTracer.java	100.00% <100.00%> (ø)
...src/main/java/io/opentelemetry/api/trace/Span.java	93.47% <100.00%> (ø)
...c/main/java/io/opentelemetry/api/trace/SpanId.java	100.00% <100.00%> (ø)
.../main/java/io/opentelemetry/api/trace/TraceId.java	100.00% <100.00%> (ø)
...in/java/io/opentelemetry/sdk/metrics/SdkMeter.java	100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[MrAlias]

I'd want to hold off on merging this until the spec provides more clarity. It could end up being the case that noop API is supposed to do this validation.

This seems reasonable 👍

[breedx-splk]

I'd want to hold off on merging this until the spec provides more clarity. It could end up being the case that noop API is supposed to do this validation.

Thanks, totally appreciate that perspective. I also commented on the issue to try and round up some momentum.

[der-eismann]

Not sure if related, but with spring-security 6.0 we stumbled upon this issue. While the space in the name is unwanted, keeping the names under 63 characters long seems to be hard with the given example of spring.security.filterchains.SecurityContextHolderAwareRequestFilter.before. Is this just about removing the log lines about the noop stuff that's currently polluting our logs?

[tsloughter]

@der-eismann it is unrelated. This is just about removing the check from the no-op API, but the constraint on length is still in the spec.

[MrAlias]

Related specification PR: open-telemetry/opentelemetry-specification#3171

[jack-berg]

@jkwatson FYI spec PR #3171 seems likely to merge and explicitly requires that noop implementations do not do any instrument name / unit validation. This is fine with me, since it makes the noop a true noop, but means that you won't be able to see the same name / unit warnings when the noop is installed versus the sdk.

[jkwatson]

🚢

[jack-berg]

These validation utils can be moved to the SDK now. checkInstrumentName probably makes most sense in SdkMeter. checkValidInstrumentUnit should be removed given that the spec has clarified that SDKs aren't supposed to do any validation on unit. You can do that removal in this PR if you want, or just move the method to AbstractInstrumentBuilder.

[jack-berg]

Bumping this @breedx-splk.

[breedx-splk]

Ok, I moved checkValidInstrumentName() to a new internal utility class in the sdk, rather than SdkMeter. This seemed a little more SRPy to me.

I did move the unit check over to the AbstractInstrumentBuilder class, and I think removal of that should be tackled in a separate PR.

Once those were moved out of the validator, the only things that were left inValidationUtil were the logging methods, so I renamed it to ApiUsageLogger.

[hriddhighosh4]

@breedx-splk Hi. It will be really helpful if we can get the changes in this PR merged.
We are using spring 3.0.2 and are getting the same error on the latest opentelemtry collector contrib image.
It was fixed in spring security 6.0.2 but we are still getiing the error.