[BUILD] Fix exported definitions when building DLL with STL

[perhapsmaple]

Partially Fixes #2202

Changes

The function definitions for MeterSelectorFactory::Create and InstrumentSelectorFactory::Create are different for the nostd and stl builds, which causes linker errors when trying to build the DLL.

Cmake build WITH_STL=OFF:

?Create@MeterSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VMeterSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VMeterSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@Vstring_view@nostd@45@00@Z

?Create@InstrumentSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@W4InstrumentType@2345@Vstring_view@nostd@45@1@Z

CMake build WITH_STL=ON:

?Create@MeterSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VMeterSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VMeterSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@V?$basic_string_view@DU?$char_traits@D@std@@@7@00@Z

?Create@InstrumentSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@W4InstrumentType@2345@V?$basic_string_view@DU?$char_traits@D@std@@@7@1@Z

Introduced a compile definition OPENTELEMETRY_STL_VERSION to the DLL build process to export the correct definitions.

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

Merging #2387 (80f31c5) into main (758687c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 80f31c5 differs from pull request most recent head 105c1a6. Consider uploading reports for the commit 105c1a6 to get more accurate results

@@           Coverage Diff           @@
##             main    #2387   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files         199      199           
  Lines        6030     6030           
=======================================
  Hits         5272     5272           
  Misses        758      758           

Files	Coverage Δ
...entelemetry/sdk/metrics/view/instrument_selector.h	100.00% <ø> (ø)
...de/opentelemetry/sdk/metrics/view/meter_selector.h	100.00% <100.00%> (ø)

[marcalff]

Thanks for the analysis and fix, see comments.

[marcalff]

LGTM, thanks for the fix.

Waiting for approval from @ThomsonTan who owns this area.

[lalitb]

I think it is better to change the MeterSelector (and so also MeterSelectorFactory) to take const std::string & instead of nostd::string_view for name, version and schema. There is no reason to use nostd::string_view in SDK surface, as we don't guarantee ABI stability there. And so just maintain single export definitions for DLL.

Edit - Same for InstrumentSelector.

[marcalff]

I think it is better to change the MeterSelector (and so also MeterSelectorFactory) to take const std::string & instead of nostd::string_view for name, version and schema. There is no reason to use nostd::string_view in SDK surface, as we don't guarantee ABI stability there. And so just maintain single export definitions for DLL.

Edit - Same for InstrumentSelector.

Thanks @lalitb

Agreed, this is a much better solution.

[marcalff]

Per @lalitb findings, opentelemetry-cpp needs to cleanup the SDK exposed interface instead, to not use nostd::string_view.

[marcalff]

Excellent, thanks for revising the patch.

Please add a section in the CHANGELOG, for breaking changes,
listing the APIs affected by the change to const std::string&.

[marcalff]

Do we need the string header as well ?
Same for every header file touched.

[lalitb]

Thanks. LGTM once the string header is included in modified header files.