[cmd/mdatagen] telemetry::metrics::my_metric::enabled has no effect

[jade-guiton-dd]

Describe the bug

As far as I can tell, the enabled flag for the internal metrics of a component (example in samplereceiver) has no functional effect. The metric will still be emitted by the Collector.

Steps to reproduce

• Change the enabled flag in the link above to false. (This flag is for the internal metric of the samplereceiver.)
• Re-run mdatagen and compare the generated files.

What did you expect to see?
I would expect some change in /cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_telemetry.go with the effect of preventing the batch_size_trigger_send metric from being emitted.

What did you see instead?
The only change is that the metric is no longer listed in documentation.md.

Proposed solution
Removing the enabled flag entirely. This would avoid misleading component authors into thinking their metric is disabled.

This may be related to #11754.

[github-actions]

Pinging code owners:

• cmd/mdatagen: @dmitryax

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[yaten2302]

Hi @jade-guiton-dd ,

Removing the enabled flag entirely. This would avoid misleading component authors into thinking their metric is disabled.

Just a suggestion, instead of removing this flag entirely, can't we add a functionality so that those metrics with enabled:false won't be emitted?

[jade-guiton-dd]

That would work too. I'm not sure if that feature would be useful enough for the effort to be worth it though (especially compared to level-based conditional enabling as in #11754).

[yaten2302]

Okay, so if we disable this, then enabled:true/false would mean nothing, right?
We'll have to remove those?
I'll raise a PR for this change, and I think it would also require an upstream issue to track the removal of the enabled flag from all the components, right?
May I create an issue for the same?

[dmathieu]

I agree that using the level flag is better than this enabled flag.
Yes, you can open a PR. I'm not sure we need another issue for all the components though. This one should be enough.

[yaten2302]

I agree that using the level flag is better than this enabled flag.

I'm not really sure what the level flag does, maybe you could give some insights/references for this 👀 ?

And, should I open a PR to completely remove the enabled flag, right?

[dmathieu]

See https://pkg.go.dev/go.opentelemetry.io/collector/config/configtelemetry#Level

[jade-guiton-dd]

See https://pkg.go.dev/go.opentelemetry.io/collector/config/configtelemetry#Level

FYI I don't think mdatagen currently has any way to specify the levels at which a metric should be emitted (that's tracked in #11754). The filtering is implemented by a list of hardcoded metric views in service.go at the moment.

And, should I open a PR to completely remove the enabled flag, right?

Yes, please.

[yaten2302]

Hi @jade-guiton-dd , in order to completely remove the enabled flag, we'll have to remove the enabled field from the metadata.go, right? and then modify the others according to this. Since, this contains a lot of structs/attributes/warnings, where the enabled field has been used and if this field isn't set, then a warning will be shown, like here

[jade-guiton-dd]

I would say that if we're removing the Enabled field entirely, we should also remove the IfEnabled and IfEnabledNotSet warnings.

But looking at the code, it looks like the bigger problem is that the internal metrics for a component (telemetry::metrics) share their config struct with the external metrics (metrics), which can actually be disabled (by default and then explicitly enabled by a component). So fully removing the Enabled field and corresponding warnings would mean not only breaking changes (since some components are using the flag and will cause a config decoding error once it's removed), but also splitting the struct in a somewhat complicated way.

So I'm reconsidering whether removing it completely is worth it. Maybe instead it would be better to consider that the flag is meaningful for internal metrics, but setting it to false isn't implemented at the moment, so mdatagen should error out in that case (or at least emit a warning if that's still too much breakage).

[yaten2302]

I would say that if we're removing the Enabled field entirely, we should also remove the IfEnabled and IfEnabledNotSet warnings.

Yes, I agree.

But looking at the code, it looks like the bigger problem is that the internal metrics for a component (telemetry::metrics) share their config struct with the external metrics (metrics), which can actually be disabled (by default and then explicitly enabled by a component). So fully removing the Enabled field and corresponding warnings would mean not only breaking changes (since some components are using the flag and will cause a config decoding error once it's removed), but also splitting the struct in a somewhat complicated way.

Yep, while I was looking at the code, I tried to edit that as well, but 1 removal created 4 new errors :)
I'll go through the code once again and check if we remove this, then how can we eliminate the new errors.

So I'm reconsidering whether removing it completely is worth it. Maybe instead it would be better to consider that the flag is meaningful for internal metrics, but setting it to false isn't implemented at the moment, so mdatagen should error out in that case (or at least emit a warning if that's still too much breakage).

Surely we can go with this, I'll try with this approach too!