Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ETW Exporter] Add Virtual destructor for TailSampler, Update Maintainer mode warnings for MSVC #1897

Merged
merged 15 commits into from
Jan 4, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jan 3, 2023

Fixes #1896

Changes

  • Add default virtual destructor for etw::TailSampler.
  • Enforce the missing virtual destructor warning as error for MSVC while building in maintainer mode.
  • The inclusion of <future> header with this warning enforced fails with below error, so suppressed the warning for this header:
    C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\pplwin.h(80,1): error C5204: 'Concurrency::details::_DefaultPPLTaskScheduler': class has virtual functions, but its trivial destructor is not virtual; instances of objects derived from this class may not be destructed correctly [D:\a\opentelemetry-cpp\opentelemetry-cpp\build\sdk\src\metrics\opentelemetry_metrics.vcxproj]
  • VS2019 v16.x reports signed/unsigned mismatch errors at few places while building in maintainer mode, fixed those errors.

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

@lalitb lalitb requested a review from a team January 3, 2023 23:51
@lalitb lalitb changed the title [ETW Exporter] Add Virtual destructor for Tail Based Sampler. [ETW Exporter] Add Virtual destructor for TailSampler. Jan 3, 2023
@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #1897 (c3d2921) into main (a343da0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1897   +/-   ##
=======================================
  Coverage   85.74%   85.74%           
=======================================
  Files         171      171           
  Lines        5243     5243           
=======================================
  Hits         4495     4495           
  Misses        748      748           
Impacted Files Coverage Δ
...lemetry/ext/http/client/curl/http_operation_curl.h 100.00% <ø> (ø)
...metrics/export/periodic_exporting_metric_reader.cc 73.92% <ø> (ø)

@lalitb lalitb marked this pull request as draft January 4, 2023 01:57
@lalitb lalitb marked this pull request as ready for review January 4, 2023 06:54
@lalitb
Copy link
Member Author

lalitb commented Jan 4, 2023

This is ready for review now.

@lalitb lalitb changed the title [ETW Exporter] Add Virtual destructor for TailSampler. [ETW Exporter] Add Virtual destructor for TailSampler, Update Maintainer mode warnings for MSVC Jan 4, 2023
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 (and for using the maintainer mode)

@lalitb lalitb merged commit 61bc860 into open-telemetry:main Jan 4, 2023
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.

[ETW Exporter] Virtual destructor missing for etw::TailSampler
2 participants