[BUILD] Revisit EventLogger deprecation

[marcalff]

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

• Removed the OPENTELEMETRY_DEPRECATED C++ annotation for EventLogger.
  • This was used to tell the compiler to emit a warning.
• Removed all the warning suppression code from the api
  • This was used to tell the compiler to suppress the warning just added, doing both makes no sense.
• Preserved OPENTELEMETRY_DEPRECATED only on entry points, without warning suppression pragma:
  • API opentelemetry::logs::Provider::GetEventLoggerProvider()
  • API opentelemetry::logs::Provider::SetEventLoggerProvider()
  • SDK opentelemetry::sdk::logs::EventLoggerProviderFactory
• Added warning suppression code in unit tests only
  • This allows to have test coverage of deprecated code, while keeping the build clear of noise.
• Documented the code as deprecated in doxygen
• Updated DEPRECATED.md to clarify the migration path

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (b646e8c) to head (f144725).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3855   +/-   ##
=======================================
  Coverage   90.05%   90.05%           
=======================================
  Files         226      226           
  Lines        7200     7200           
=======================================
  Hits         6483     6483           
  Misses        717      717           

Files with missing lines	Coverage Δ
api/include/opentelemetry/logs/event_logger.h	90.00% <ø> (ø)
...include/opentelemetry/logs/event_logger_provider.h	100.00% <ø> (ø)
api/include/opentelemetry/logs/noop.h	79.32% <ø> (ø)
api/include/opentelemetry/logs/provider.h	100.00% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lalitb]

I'm not fully clear on the intent behind these changes. My understanding is that anyone using EventLogger should receive a deprecation warning. That was broken in #3845, and from what I can tell, this change doesn't restore that behavior either - so what's the purpose of removing the warning?

[marcalff]

@lalitb

The main problem is to decide clearly what opentelemetry-cpp wants to do with deprecation for EventLogger in particular.

Option 1

We mark code with OPENTELEMETRY_DEPRECATED, causing compiler warnings.

Anyone who uses a deprecated class gets a warning.

This includes:

• unit tests in opentelemetry-cpp
• noop providers
• the Provider class as well, because of Get/Set EventLoggerProvider.

This creates a problem for applications, even when not using EventLogger, because the simple fact of including "provider.h" (which is required and can not be avoided) causes the build to fail with -Wall -Werr.

Option 2

We do not mark code with OPENTELEMETRY_DEPRECATED, and only document that some classes are deprecated.

This is what this PR does.

---

Option 1 is what was done since the beginning with #3285, but recently issue #3835 was reported complaining (correctly) that there is no way to have a clean build, and this was "fixed" by suppressing all warnings with #3845.

If we prefer to have warnings, we need to rollback #3845.

If we prefer to not have warnings, we need this PR to clean up the code.

Leaving both OPENTELEMETRY_DEPRECATED and pragmas to disable deprecation warnings makes no sense.

The root cause of all this is that, when a deprecation is added, there should be a migration path for users to take and avoid using the deprecated code.

In this case, because the Get/Set EventLoggerProvider methods are inline, part of the API, and located in a mandatory header file, there is no way for user code to avoid compiling deprecated code, so there is no way to resolve deprecation warnings if present.

We can not (even with conditional compilation) opt out for these methods either, as this will break the ABI for version 1.

[lalitb]

Thanks @marcalff. I agree that having both OPENTELEMETRY_DEPRECATED and pragma suppression together on the same class makes no sense - that definitely needs fixing.

However, I don't think this is strictly Option 1 vs Option 2. There's an Option 3:

The concern with Option 1 is that including provider.h causes deprecation warnings even for users who never use EventLogger. But that's only true if we put OPENTELEMETRY_DEPRECATED on the class declarations - which, combined with MSVC warning on declarations, forces warnings on everyone who includes the header.

The fix is to put OPENTELEMETRY_DEPRECATED only on GetEventLoggerProvider() and SetEventLoggerProvider() methods - not on the classes. The pragma push/pop around these method declarations in the header suppresses the MSVC declaration-site warning, so just including provider.h won't cause any warning. But when a user actually calls GetEventLoggerProvider() or SetEventLoggerProvider() in their own code, the deprecation warning fires at the call site - on GCC/Clang and intended on MSVC via scoped pragma suppression. That's exactly the behavior we want.

In this case, because the Get/Set EventLoggerProvider methods are inline, part of the API, and located in a mandatory header file, there is no way for user code to avoid compiling deprecated code, so there is no way to resolve deprecation warnings if present.

This is true for MSVC (which warns at declaration), but pragma push/pop around the method declarations in the header handles exactly that - it suppresses the MSVC declaration-site warning so including the header is clean. On GCC/Clang, OPENTELEMETRY_DEPRECATED on a method only warns at the call site, not the declaration, so there's no issue at all. The pragma around the declaration isn't defeating the purpose here - it's specifically scoped to suppress the MSVC declaration quirk. User code that actually calls GetEventLoggerProvider() is outside the pragma scope, so the deprecation warning correctly fires at the call site.

So:

• EventLogger/EventLoggerProvider classes: Remove OPENTELEMETRY_DEPRECATED and pragma push/pop (as this PR does)
• Noop/SDK implementations: Remove OPENTELEMETRY_DEPRECATED
• GetEventLoggerProvider()/SetEventLoggerProvider() in provider.h: Keep OPENTELEMETRY_DEPRECATED on these methods, with pragma push/pop around the declarations (for MSVC)

This gives users a clean build when they only use LoggerProvider::GetLogger(), and a clear deprecation warning when they actually call the deprecated Event API entry points. No ABI break, no need for conditional compilation.

We could also enforce this in CI by adding a small compile test target that compiles with -Werror=deprecated-declarations (GCC/Clang). or /we4996 (MSVC), verifying that including provider.h and using only GetLoggerProvider() compiles clean, while calling GetEventLoggerProvider() produces a compile error. This would be a new addition though.

[marcalff]

@lalitb

Thanks for the comments.

I implemented:

• Preserved OPENTELEMETRY_DEPRECATED only on entry points, without warning suppression pragma:
  • API opentelemetry::logs::Provider::GetEventLoggerProvider()
  • API opentelemetry::logs::Provider::SetEventLoggerProvider()
  • SDK opentelemetry::sdk::logs::EventLoggerProviderFactory

which is basically option 3.

The claim that MSVC raises a deprecation warning at declaration site is very weird to me, so I wanted to see this for myself to confirm, and used deprecated provider set/get methods without the warning suppression code.

It turns out, from the CI logs even on windows, that no warning are generated, unless I missed something in build logs.

Starting fresh with all the related cleanup done in EventLogger classes, code like:

  OPENTELEMETRY_DEPRECATED static nostd::shared_ptr<EventLoggerProvider>
  GetEventLoggerProvider() noexcept
  {
    std::lock_guard<common::SpinLockMutex> guard(GetLock());
    return nostd::shared_ptr<EventLoggerProvider>(GetEventProvider());
  }

behaves as it should.

I suspect that the following helps:

• the GetEventLoggerProvider() prototype, in particular the return type, no longer uses deprecated classes
• the GetEventLoggerProvider() implementation, inlined in the header file, no longer uses deprecated classes

so that no warnings are generated at declaration time, even on MSVC.

Please double check with a local build if possible.

[ThomsonTan]

For MSVC, that’s generally true—especially for deprecation warnings (e.g., C4996), which are typically emitted at the point of use (the call site) rather than at the declaration.

so that no warnings are generated at declaration time, even on MSVC.

[lalitb]

LGTM. I don't have the windows machine to validate it (moved to MacOS) - but good to have a track issue for adding the compile-contract CI test to validate this fix.

[lalitb]

Though I do see a separate blocker: this PR changes MSVC maintainer warning policy for C4996 (/wd4996 -> /w34996 under /WX), which causes unrelated existing deprecations (examples/grpc semconv RPC constants and sscanf configuration parser) to fail Windows CI. Could we either

• revert the C4996 policy change in this PR (preferred, to keep scope focused), and open a follow-up tracking issue to add a dedicated deprecation contract test
• or include fixes for all current C4996 callsites in the same PR?

[marcalff]

Though I do see a separate blocker: this PR changes MSVC maintainer warning policy for C4996 (/wd4996 -> /w34996 under /WX), which causes unrelated existing deprecations (examples/grpc semconv RPC constants and sscanf configuration parser) to fail Windows CI. Could we either

* revert the C4996 policy change in this PR (preferred, to keep scope focused), and open a follow-up tracking issue to add a dedicated deprecation contract test

* or include fixes for all current C4996 callsites in the same PR?

This was an attempt at more cleanup but it failed, reverting it.

Cleaning up semconv is a separate problem, see #3890