Switch from MPark variant to absl::variant as default

[maxgolov]

Fixes #572 #733

Changes

Why:

• MPark Variant is:
  a) Boost-licensed not Apache License v2 (forcing us to depend on Boost License unnecessarily)
  b) Does not compile with Visual Studio 2015 at all.

How to fix:

• use absl::variant , because it is:
  a) Apache License v2
  b) It compiles well with Visual Studio 2015 Update 2.

UPDATE 1: there is a change needed in common::AttributeValue to re-add const char *, because neither std::variant nor absl::variant provide an implicit conversion of that type to string_view. What happens then, it by-default gets converted to bool. So any static initializer in the map, for a pair such as {{"key, "value"}} is gonna break, by assigning the variant value with type bool instead of string_view. I'm implementing a fix to resolve this. Related issue #733 #734

UPDATE 2: unfortunately we also need to add const char * attribute value constructor to OwnedAttributeValue since otherwise it'd break the SDK ResourceAttribute implementation. In most cases the exporters do not have to deal with const char * because transform from AttributeValue variant to OwnedAttributeValue variant presently implies a memcpy of const char * into owning std::string object. However, I did adjust all default exporters to potentially deal with const char *. This is 3 lines of code per exporter, and I would argue the maintenance cost of that is zero.

UPDATE 3: - as discussed, I removed const char * from OwnedAttributeValue. I also refactored ResourceAttribute to use a map that allows to transform incoming const char * attributes assigned using initializer lists into std::string.

• CHANGELOG.md updated for non-trivial changes
• Changes in public API reviewed

I added a separate document that explains the process of updating Abseil snapshot in this PR #797

[lalitb]

LGTM. Thanks for the cleanup. Though CI is failing for now.
I think it should be fine to break ABI for now, while we are still not GA :)

[ThomsonTan]

Probably #include "absl/base/config.h" should be #include "../types.config.h" in api/include/opentelemetry/nostd/absl/types/variant.h. Does this only fail CI build? @maxgolov

[maxgolov]

@lalitb @ThomsonTan ..

Folks, please don't merge it yet. I pushed the latest update. Unfortunately there are two outstanding issues:

• it appears that absl::variant has the same issue we discussed here that happens with std::variant. Avoid pointer to reference to temporaries in SetResource API #733 potential fix for std::variant implicit conversion in C++17 STL #734

Because I see the same EXACT tests failing now with Abseil, because the variant does not copy temporary 😢 ... OR treats it as bool type, whatever it is, it messes up the outcome.

Screenshot:

• the other second issue is shipping absl/ directory inside api/include/nostd. I might need to fix paths in our snapshot of Abseil, to use paths relative to api/include/opentelemetry , because I am not sure yet how we can add include directory of api/include/opentelemetry/nostd to dependent projects (which would be required for #include "absl/base/config.h" to work, I temporarily added a hack in root-level CMakeLists.txt` to run the tests, but that hack is not good as a permanent solution).

There is also something related to being able to build with original Abseil. What I was trying to avoid is linking to original Abseil. Because Abseil variant implementation otherwise works great as a header-only library. The only problem with that is implementation of exception handler.

I need a bit more work on that. I might need an advise from @owent how to export our local Abseil headers, in case if we bundle Abseil within, not needing an external snapshot of it.. to keep the entire API still pure header-only. That's a requirement from someone else. And I need the same (header-only API) for ETW exporter build. It would be sad to lose that pure header-only feature of API target.

[maxgolov]

I can try to fix the tests by making sure we pass std::string rather than const char *. But we kinda have a long-term issue on API surface now, that's maybe because we did not add const char * to variant... 🤷 ... If we had it on AttributeValue, then I guess we wouldn't have had the implicit conversion to bool issue?? I can try patching that..

[maxgolov]

I got down to only 5 test failures by re-adding const char * to AttributeValue. It solves the original issue described in #733 #734. The change is NOT SMALL, and it's been a struggle so far. I'm pushing the latest update. The only remaining tests that are failing are now isolated to resources.ResourceTest.* , all other tests are now passing.

Basically, for both - std::variant since C++17, and for absl::variant, in order to provide convenient initialization from temporary const char *, we have to re-add const char * explicitly into common::AttributeValue variant on API surface. Because otherwise without that explicit conversion - it get implicitly converted to bool instead of nostd::string_view..

Which breaks a simple construct like { { "key, "value } } in static initializer lists. I don't think this is good dev experience at all.

Refreshed PR with latest stuff. Only 5 tests are now failing, down from 10. Hopefully I can sort out the rest of them in a day.

[owent]

@lalitb @ThomsonTan ..

Folks, please don't merge it yet. I pushed the latest update. Unfortunately there are two outstanding issues:

• it appears that absl::variant has the same issue we discussed here that happens with std::variant. Avoid pointer to reference to temporaries in SetResource API #733 potential fix for std::variant implicit conversion in C++17 STL #734

Because I see the same EXACT tests failing now with Abseil, because the variant does not copy temporary 😢 ... OR treats it as bool type, whatever it is, it messes up the outcome.

Screenshot:

• the other second issue is shipping absl/ directory inside api/include/nostd. I might need to fix paths in our snapshot of Abseil, to use paths relative to api/include/opentelemetry , because I am not sure yet how we can add include directory of api/include/opentelemetry/nostd to dependent projects (which would be required for #include "absl/base/config.h" to work, I temporarily added a hack in root-level CMakeLists.txt` to run the tests, but that hack is not good as a permanent solution).

There is also something related to being able to build with original Abseil. What I was trying to avoid is linking to original Abseil. Because Abseil variant implementation otherwise works great as a header-only library. The only problem with that is implementation of exception handler.

I need a bit more work on that. I might need an advise from @owent how to export our local Abseil headers, in case if we bundle Abseil within, not needing an external snapshot of it.. to keep the entire API still pure header-only. That's a requirement from someone else. And I need the same (header-only API) for ETW exporter build. It would be sad to lose that pure header-only feature of API target.

We could add these cmake scripts below into api/CMakeLists.txt to replace include_directories(api/include/opentelemetry/nostd) .

if(WITH_ABSEIL)
  target_compile_definitions(opentelemetry_api INTERFACE HAVE_ABSEIL)
  target_link_libraries(opentelemetry_api INTERFACE ${CORE_RUNTIME_LIBS})
else()
  target_include_directories(
    opentelemetry_api
    INTERFACE
      "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include/opentelemetry/nostd>"
      "$<INSTALL_INTERFACE:include/opentelemetry/nostd>")
endif()

[maxgolov]

@owent - I just pushed an update to use relative paths for local Abseil. I still leave an option for end-users / devs to decide if they want the full Abseil, e.g. WITH_ABSEIL. I like your suggestion. I do not exactly like what I did with relative paths in latest...

[owent]

@owent - I just pushed an update to use relative paths for local Abseil. I still leave an option for end-users / devs to decide if they want the full Abseil, e.g. WITH_ABSEIL. I like your suggestion. I do not exactly like what I did with relative paths in latest...

I like the WITH_ABSEIL option to allow me to use my own abseil.But I don't see any options for bazel. Should opentelemetry_api use abseil imported by gRPC when using bazel?

[maxgolov]

Should opentelemetry_api use abseil imported by gRPC when using bazel?

I think it's a good call. Because gRPC uses Abseil anyways, if we are building SDK optimized for that same Abseil, we do not bloat with unnecessary local copy. What I am not certain --- since this is in API, and since this is in the main part of API -- common::AttributeValue used for Spans, Events, etc.. How do we pre-package the necessary Abseil headers for the customer to use, OR do we need to document that the customer themselves need to take a dependency on that exact same version of Abseil that gRPC does? I am not sure how we neatly document that..

Also another interesting item, maybe we could have also used absl::string_view. Then we consolidate more basic types within the same library, less bloat (but I would like to cover string_view in a separate PR).

[lalitb]

Basically, for both - std::variant since C++17, and for absl::variant, in order to provide convenient initialization from temporary const char *, we have to re-add const char * explicitly into common::AttributeValue variant on API surface. Because otherwise without that explicit conversion - it get implicitly converted to bool instead of nostd::string_view..

I got down to only 5 test failures by re-adding const char * to AttributeValue. It solves the original issue described in #733 #734. The change is NOT SMALL, and it's been a struggle so far. I'm pushing the latest update. The only remaining tests that are failing are now isolated to resources.ResourceTest.* , all other tests are now passing.

Basically, for both - std::variant since C++17, and for absl::variant, in order to provide convenient initialization from temporary const char *, we have to re-add const char * explicitly into common::AttributeValue variant on API surface. Because otherwise without that explicit conversion - it get implicitly converted to bool instead of nostd::string_view..

Which breaks a simple construct like { { "key, "value } } in static initializer lists. I don't think this is good dev experience at all.

Refreshed PR with latest stuff. Only 5 tests are now failing, down from 10. Hopefully I can sort out the rest of them in a day.

FYI - this issue has been fixed in later versions of std::variant as below:

Resolution is described here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0608r3.html

And this issue (P0608R3) is fixed in:
clang 9.0 - https://libcxx.llvm.org/docs/Cxx2aStatus.html,
GCC 10.1 - https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2020
microsoft/STL - microsoft/STL#1629

I remember there were ownership issues pointed out by @pyohannes earlier for adding support for const char * in AttributeValue. Will try to dig more on that. Should we switch back to default mpart::variant in that case, as ideally, the fix should come from these libraries?

[lalitb]

Will try to dig more on that.

The earlier discussion around this context:

#519 (comment)

[maxgolov]

@pyohannes @lalitb - could you have a quick look?

Latest changes address the issues we discussed during SIG meeting:

• API layer common::AttributeValue now has const char *.

• SDK layer OwnedAttributeValue - does not need const char * anymore. It stores value in std::string. Visitor pattern converter helps to transform from const char * (on API surface) to std::string (in SDK).

• I refactored Resource Attribute + revamped the existing helper class AttributeMap, that one now facilitates the visitor pattern transform of incoming AttributeValue(const char *) (API layer) into OwnedAttributeValue(std::string) (SDK layer). Resource Attributes now safely be initialized from initializer lists (with C-strings on them), and internally they store all values as std::string.

[lalitb]

Thanks, @maxgolov - Looks good to me now.

@pyohannes, @ThomsonTan - would you like to review changes specific to const char * for common::AttributeValue, and sdk::common::OwnedAttributeType as what we discussed in yesterday's meeting?

[maxgolov]

@owent - once this PR merged, could you please share some instructions, did we need this for MinGW? I think maybe we're not gonna be needing this code anymore. If we are building with HAVE_ABSEIL in future, we can demand that the implementation of ThrowBadVariantAccess comes from Abseil lib linked into the project. That way we can remove it from core.cc here.. Basically, I'm no longer using this exception handler for our own Abseil private variant, and most likely the code between lines 10 to 36 could be simply removed (in a separate PR 😄 ).

[maxgolov]

@pyohannes @ThomsonTan @jsuereth

I believe I have addressed ALL the questions / issues. Would appreciate feedback on this. Integrating it should allow me to set up the build loops for vs2015-vs2017-vs2019, using Abseil Variant. As well as all tests for STDLIB are now passing, so I can add a CI loop with STDLIB (for statically linking using C++20 classes) for other platforms as well. Now we have all issues related to const char * conversion (finally) sorted out. For both - Abseil and STL/STD variant.

New commits comes in.

[ThomsonTan]

The PR and the const char * change LGTM.