Skip to content

Conversation

owent
Copy link
Member

@owent owent commented Aug 15, 2025

Fixes #3588

Changes

Please provide a brief description of the changes here.

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

@Copilot Copilot AI review requested due to automatic review settings August 15, 2025 11:36
@owent owent requested a review from a team as a code owner August 15, 2025 11:36
Copy link

netlify bot commented Aug 15, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 4123b34
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/68a37223638a2500089df1ab

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes duplicated deprecated warnings in the logs headers by adding pragma directives to suppress deprecation warnings around code that uses deprecated APIs. The changes ensure that deprecation warnings are properly silenced during compilation across different compiler platforms (MSVC, GCC, and Clang).

  • Adds compiler-specific pragma directives to suppress deprecation warnings
  • Wraps usage of deprecated APIs with warning suppression blocks
  • Ensures consistent warning suppression across MSVC, GCC, and Clang compilers

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
api/include/opentelemetry/logs/provider.h Adds pragma directives around NoopEventLoggerProvider instantiation to suppress deprecation warnings
api/include/opentelemetry/logs/noop.h Adds pragma directives around NoopEventLoggerProvider constructor to suppress deprecation warnings

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

# pragma GCC diagnostic pop
# elif defined(__clang__) || defined(__apple_build_version__)
# pragma clang diagnostic pop
# endif
Copy link

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pragma directive blocks are duplicated across multiple files. Consider creating a macro or header file to define OPENTELEMETRY_SUPPRESS_DEPRECATED_START and OPENTELEMETRY_SUPPRESS_DEPRECATED_END to reduce code duplication and improve maintainability.

Suggested change
# endif
OPENTELEMETRY_SUPPRESS_DEPRECATED_START
static nostd::shared_ptr<EventLoggerProvider> provider(new NoopEventLoggerProvider);
return provider;
OPENTELEMETRY_SUPPRESS_DEPRECATED_END

Copilot uses AI. Check for mistakes.

# elif defined(__clang__) || defined(__apple_build_version__)
# pragma clang diagnostic pop
# endif

Copy link

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pragma directive blocks are duplicated across multiple files. Consider creating a macro or header file to define OPENTELEMETRY_SUPPRESS_DEPRECATED_START and OPENTELEMETRY_SUPPRESS_DEPRECATED_END to reduce code duplication and improve maintainability.

Suggested change
OPENTELEMETRY_SUPPRESS_DEPRECATED_START
NoopEventLoggerProvider() : event_logger_{nostd::shared_ptr<EventLogger>(new NoopEventLogger())}
{}
OPENTELEMETRY_SUPPRESS_DEPRECATED_END

Copilot uses AI. Check for mistakes.

Copy link

codecov bot commented Aug 15, 2025

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3592      +/-   ##
==========================================
+ Coverage   90.02%   90.03%   +0.02%     
==========================================
  Files         220      220              
  Lines        7069     7069              
==========================================
+ Hits         6363     6364       +1     
+ Misses        706      705       -1     
Files with missing lines Coverage Δ
api/include/opentelemetry/logs/noop.h 77.78% <ø> (ø)
api/include/opentelemetry/logs/provider.h 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nikhilbhatia08
Copy link
Contributor

nikhilbhatia08 commented Aug 15, 2025

@owent The same test of CI / Bazel on MacOS fails on my pull request too with the same log, sorry but I'm not able to understand why it fails, can you please tell the fix for it. Thanks !

@lalitb
Copy link
Member

lalitb commented Aug 15, 2025

@owent The same test of CI / Bazel on MacOS fails on my pull request too with the same log, sorry but I'm not able to understand why it fails, can you please tell the fix for it. Thanks !

Not for sure, but the bazel cache could be corrupted here. If so, a nit PR deleting the cache should fix it. I believe the PR needn't be merged, just run on the main build to clean the cache. Or else, provide a custom CI action (through workflow_dispatch) to let maintainers delete the cache manually on such errors.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

@dbarker dbarker changed the title Remove dumplicated deprecated warnings in logs headers Remove duplicated deprecated warnings in logs headers Aug 18, 2025
@marcalff marcalff changed the title Remove duplicated deprecated warnings in logs headers [BUILD] Remove duplicated deprecated warnings in logs headers Aug 18, 2025
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@lalitb
Copy link
Member

lalitb commented Aug 18, 2025

The only limitation with this change is that anyone using EventLogger won’t see a deprecation warning, so they may never realize it’s deprecated. That said, I think the current PR is still the best path forward. Splitting EventLogger into its own header could address this, but that would be an API-breaking change.

@marcalff
Copy link
Member

The only limitation with this change is that anyone using EventLogger won’t see a deprecation warning, so they may never realize it’s deprecated. That said, I think the current PR is still the best path forward. Splitting EventLogger into its own header could address this, but that would be an API-breaking change.

In provider.h, warnings are silenced only in the implementation of GetEventProvider(), because it uses deprecated code, which was the reported issue.

Not tested, but I think anyone calling GetEventProvider() will still see a deprecation warning, because warnings are raised when using a deprecated api, not just when declaring an api deprecated..

@marcalff
Copy link
Member

Seen in CI:

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/logs/provider_test.cc:90:23: warning: 'GetEventLoggerProvider' is deprecated [-Wdeprecated-declarations]
   90 |   auto tf = Provider::GetEventLoggerProvider();
      |                       ^
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/logs/provider.h:57:3: note: 'GetEventLoggerProvider' has been explicitly marked deprecated here
   57 |   OPENTELEMETRY_DEPRECATED static nostd::shared_ptr<EventLoggerProvider>

So, calling deprecated api Provider::GetEventLoggerProvider() still raises warnings, as expected.

@marcalff marcalff merged commit d3ad151 into open-telemetry:main Aug 18, 2025
70 checks passed
malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request Aug 18, 2025
[BUILD] Remove duplicated deprecated warnings in logs headers (open-telemetry#3592)
@owent owent deleted the remove_deprecated_warnings_in_log_headers branch September 4, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unavoidable EventLogger Deprecated Warning

5 participants