Add check for duplicate instrument names

[ocelotl]

Fixes #2142

This PR adds the checking for duplicate instrument names in the API. Others may disagree and would consider this belongs in the SDK, please leave your comments ✌️

The issue includes this question:

Which instrument fields are considered non-identifying (description?)

I think that since the spec only requires that the instrument names are not repeated, we should be concerned with the name attribute of instruments only.

[codeboten]

Change overall looks good, just one question

[srikanthccv]

Also look at open-telemetry/opentelemetry-specification#2270

[srikanthccv]

I would suggest we hold this on until we get clarification the linked spec discussion to avoid the rework and confusion.

[codeboten]

@srikanthccv 2317 has merged, can this PR be moved forward?

[srikanthccv]

Yes, thanks for pointing out. I believe this PR needs to be updated to confirm with latest spec changes. We don't want to throw an exception anymore in case of duplicate registration and should return a valid instrument.

[ocelotl]

@srikanthccv, addressed your changes, please take a look ✌️

[srikanthccv]

We should also consider the views configuration before we warn the conflicting instrument, right?

[codeboten]

Since the check for duplicate instrument must take views into considerations, the checks should probably be moved into the SDK.

[aabmass]

The requirements I see in the API spec for this are:

1. When more than one Instrument of the same name is created for identical Meters, denoted duplicate instrument registration, the implementation MUST create a valid Instrument in every case.

2. When more than one distinct Instrument is registered with the same name for identical Meters, the implementation SHOULD emit a warning to the user informing them of duplicate registration conflict(s).

My take is we should implement requirement 2 in the API as a logged warning (which this PR does), but it should not throw an exception. The SDK should allow views to reconfigure this as @codeboten pointed out.

What do you think @codeboten?

[aabmass]

@ocelotl I think the PR description is out of date, can you update?

This PR intentionally raises an exception in the API.

[codeboten]

@aabmass I guess it's a bit confusing, because the SDK specification says that if a view exists with a description, then this should avoid the warning. Unless we're saying we should have the warning anyways, and subsequent warnings could be avoided?

If the potential conflict involves multiple description properties, setting the description through a configured View SHOULD avoid the warning.
If the potential conflict involves instruments that can be distinguished by a supported View selector (e.g., instrument type) a View recipe SHOULD be printed advising the user how to avoid the warning by renaming one of the conflicting instruments.
Otherwise (e.g., use of multiple units), the implementation SHOULD pass through the data by reporting both Metric objects.

[ocelotl]

@codeboten @aabmass @srikanthccv please consider merging this PR and addressing the resolution of conflicts via views in a separate PR (one that would solve #2509).

[codeboten]

@codeboten @aabmass @srikanthccv please consider merging this PR and addressing the resolution of conflicts via views in a separate PR (one that would solve #2509).

I'm ok with this plan.

[ocelotl]

We should also consider the views configuration before we warn the conflicting instrument, right?

Not in this PR, we will implement that in a subsequent one when we solve #2558

[ocelotl]

@aabmass @srikanthccv please take a look ✌️

[aabmass]

Not in this PR, we will implement that in a subsequent one when we solve #2558

correct me if I'm wrong, but won't that require re-implementing this work in the SDK? What's your plan, to just override the _check_instrument_id() in the SDK meter provider?