[telemetry] Add disabled flag on each telemetry type, and sync logger on shutdown.

[atoulme]

Description:
Add disabled flag on each telemetry type, and sync logger on shutdown.

Remove TODO and apply sync on telemetry shutdown

[djaglowski]

Looks like the test failures may be related to the change.

[atoulme]

I'll take a look, thanks.

[atoulme]

Yes, I'm looking more into it. There is a bit of an issue with how we manage situations where we test with this logger because stderr is not available from within tests.
I tried to disable going to stderr with an empty array, but in that case the default applies.
I have some code towards disabling logging explicitly (level: none). It expands the scope of the PR a bit and I have at least one test that depends on logger emitting at least once that fails. I'll keep looking.

[atoulme]

It turns out our tests here would fail because stderr is set by default, if no value is provided. I have developed therefore a different approach for testing: creating a log level of none where we just return a zap.NewNop() instead of building the logger.

With that change, and applying that change to the yaml config files used in tests, we get all tests to pass.

[codecov]

Codecov Report

Patch coverage: 96.55% and project coverage change: -0.14 ⚠️

Comparison is base (105e5ae) 90.99% compared to head (f2c3be6) 90.86%.

❗ Current head f2c3be6 differs from pull request most recent head 1207721. Consider uploading reports for the commit 1207721 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7094      +/-   ##
==========================================
- Coverage   90.99%   90.86%   -0.14%     
==========================================
  Files         293      291       -2     
  Lines       14268    14072     -196     
==========================================
- Hits        12983    12786     -197     
  Misses       1017     1017              
- Partials      268      269       +1     

Impacted Files	Coverage Δ
service/telemetry/config.go	100.00% <ø> (ø)
service/service.go	66.40% <85.71%> (-1.54%)	⬇️
otelcol/unmarshaler.go	100.00% <100.00%> (ø)
service/telemetry/telemetry.go	86.15% <100.00%> (+2.82%)	⬆️

... and 12 files with indirect coverage changes

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

dismiss my review since significant changes were made, will look tomorrow for the new approach

[bogdandrutu]

It turns out our tests here would fail because stderr is set by default, if no value is provided. I have developed therefore a different approach for testing: creating a log level of none where we just return a zap.NewNop() instead of building the logger.

Do I understand correctly that we added the new public API just to fix tests?

[atoulme]

Do I understand correctly that we added the new public API just to fix tests?

I guess there isn't any way to disable logging entirely, so maybe there's value to this beyond tests here.

[bogdandrutu]

@codeboten do we go with "Enable" or "Disable" int he config otep?

[codeboten]

@codeboten do we go with "Enable" or "Disable" int he config otep?

@bogdandrutu the otep currently proposes disabled

[atoulme]

I didn't know there was an otep. Got a link?

Mind approving the PR?

[dmitryax]

We don't use disable anywhere in the collector repos. I don't think we should start using disabled without planning the migration if we want to migrate. I commented on the Otep https://github.com/open-telemetry/oteps/pull/225/files#r1139485969

[jpkrohling]

Just to record somewhere: I prefer "enabled" instead of "disabled", as "disabled" requires a double-negative to have something activated (disabled=false means enabled).

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[atoulme]

Trying to understand what is blocking there. Are we stuck on disabled/enabled?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[dmitryax]

Trying to understand what is blocking there. Are we stuck on disabled/enabled?

I think so. If we want to be consistent within Collector, we need to use enabled. But if the Configuration SIG chooses a different strategy, we would need to change it in all the places. Having one more of them is probably not that critical. I'd say we can go with enabled here. @codeboten @jpkrohling @bogdandrutu WDYT?

[atoulme]

I think enabled makes more sense. To keep backwards compatibility, we should assume enabled : true by default. The issue with that is we then need to use a pointer boolean, which earlier in this PR gave pause to Bogdan. We use a pointer boolean to find whether someone set the boolean expressly to false or if the value is not set (nil pointer).

[dmitryax]

I think enabled makes more sense. To keep backwards compatibility, we should assume enabled : true by default. The issue with that is we then need to use a pointer boolean, which earlier in this PR gave pause to Bogdan. We use a pointer boolean to find whether someone set the boolean expressly to false or if the value is not set (nil pointer).

We can keep backward compatibility for end users without pointer. Default values are set https://github.com/open-telemetry/opentelemetry-collector/blob/main/otelcol/unmarshaler.go#L43, not ideal, but that's what we have

[jpkrohling]

I agree that we should pursue enabled instead of disabled unless absolutely necessary.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[tigrannajaryan]

We have logs.level and metrics.level. metrics.level already supports none. We can add none to logs.level too and then enabled/disabled won't be necessary anymore.

[atoulme]

We have logs.level and metrics.level. metrics.level already supports none. We can add none to logs.level too and then enabled/disabled won't be necessary anymore.

That was discussed and was my first array into this work, but unfortunately there was pushback as maintainers said they want log level to be zap log levels.
See this commit: a7ac1b1

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.