Add Instrument.IsEnabled per specs

[bogdandrutu]

This PR is based on open-telemetry/opentelemetry-specification#4063

In this PR, the async instruments are ignored because open-telemetry/opentelemetry-specification#4200, if that is not what we want I will add support for async instruments in a later PR.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bogdandrutu / name: Bogdan Drutu (b6ed797)

[dashpole]

Looks like the status is still Development: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#enabled.

We will need to wait for this to go stable before we can add it to the stable API.

[bogdandrutu]

Looks like the status is still Development: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#enabled.
We will need to wait for this to go stable before we can add it to the stable API.

I am extremely confused by this, because for logs it is also development, see
https://github.com/open-telemetry/opentelemetry-specification/blob/e9a2b004ccf547871c5208aa9fff3797a84549e8/specification/logs/bridge-api.md?plain=1#L135 and you can see

opentelemetry-go/log/logger.go

Line 52 in 9339b21

// concurrently.

or that is ok because log is not stable package?

[bogdandrutu]

@dashpole will work on that stability, but before that can you tell me if you prefer Enabled (like you have in log) or IsEnabled (probably log needs to change).

[dashpole]

@pellared @MrAlias

It does look like we will need to stabilize or remove Enabled from the Logs API.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (9339b21) to head (b6ed797).
Report is 172 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5768     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        272     272             
  Lines      22759   22819     +60     
=======================================
+ Hits       19253   19283     +30     
- Misses      3166    3194     +28     
- Partials     340     342      +2     

see 8 files with indirect coverage changes

[dashpole]

I have a slight preference for IsEnabled(), but that would require changing the logs api as well.

[pellared]

It does look like we will need to stabilize or remove Enabled from the Logs API.

Stabilizing of Logger.Enabled is part of https://github.com/orgs/open-telemetry/projects/43.

I have a slight preference for IsEnabled(), but that would require changing the logs api as well.

In such case, we may also want to update the specification as well.

In Trace API we have methods like IsValid, IsSampled, IsRemote.

On the other hand, the Go standard library has e.g. Enabled in log/slog and in net/http the "bool methods/fields" have no prefixes (like https://pkg.go.dev/net/http#Response.ProtoAtLeast).

Personally, as rule of thumb, I find adding Is prefix not Go idiomatic, similarly like adding Get prefix is also not seen as idiomatic. Reference: https://go.dev/doc/effective_go#Getters. However, probably consistency in API packages is more important (ship has sailed).

[bogdandrutu]

In such case, we may also want to update the specification as well.

The choice between IsEnabled vs Enabled is allowed at SIG level since this is language canonical way of doing things. In java this most likely will be "isEnabled" because follows things like https://docs.oracle.com/javase/8/docs/api/java/util/List.html#isEmpty--.

[bogdandrutu]

In Trace API we have methods like IsValid, IsSampled, IsRemote.

Because of this I would suggest "IsEnabled".

[cijothomas]

Suggested change

	// recording measurements. The returned value cannot be cached since it
	// recording measurements. The returned value should not be cached since it

felt this is more apt, but feel free to ignore this comment, as I don't have full context on Otel Go!

[codeboten]

To move this PR along, would it be enough to add an optional experimental Enabler (or whatever this would be called) interface to be implemented by instruments in the SDK only and not the API?

This would allow the Collector to use this functionality and validate that the interface works for our use-case

[dashpole]

I don't think we can make any changes to the public API or SDK interfaces while it is experimental, so some ideas are:

• Introduce new, permanently-experimental modules to contain experimental interfaces and implementations (e.g. go.opentelemetry.io/otel/metric/experimental and go.opentelemetry.io/otel/sdk/metric/experimental).
• Implement IsEnabled as a wrapper around the current API and SDK in an internal module in the collector.

[codeboten]

@dashpole looking at this current PR, I'm not sure how the functionality would be implemented in a wrapper in the collector as the check depends on internals for all instruments.

The other option sounds like the correct one, as any future functionality will need to be exposed in a similar way.

[MrAlias]

I would prefer not maintaining a duplicate package to support an experimental PoC of a feature. Why can't the fork this already exists in be used if a user wants to try this out?

[pellared]

Blocking as this is proposing an experimental API to a stable package.

[pellared]

Changing to draft as there is no work here.

[pellared]

Closing per #6016