potential fix for std::variant implicit conversion in C++17 STL

[lalitb]

Fixes ##733

Changes

@maxgolov This is not for merge, but to showcase the potential changes needed to fix #733, As mentioned in the comment there, this is not related to resource sdk.

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

[codecov]

Codecov Report

Merging #734 (b12a746) into main (840701b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #734   +/-   ##
=======================================
  Coverage   96.00%   96.01%           
=======================================
  Files         176      176           
  Lines        7183     7199   +16     
=======================================
+ Hits         6896     6912   +16     
  Misses        287      287           

Impacted Files	Coverage Δ
exporters/ostream/test/ostream_log_test.cc	100.00% <100.00%> (ø)
exporters/ostream/test/ostream_span_test.cc	100.00% <100.00%> (ø)
sdk/src/resource/resource.cc	84.00% <100.00%> (+1.39%)	⬆️
sdk/test/resource/resource_test.cc	93.69% <100.00%> (+0.83%)	⬆️
sdk/test/trace/tracer_test.cc	99.45% <100.00%> (+<0.01%)	⬆️

[maxgolov]

I'm Okay with this as a temporary measure. I think we could have handled it in the AttributeValue class. I think the issue is that since it's a non-owning and always creates a string_view - this causes some odd issues with temporary arrays of chars..

[lalitb]

I'm Okay with this as a temporary measure. I think we could have handled it in the AttributeValue class.

This is not related to AttributeValue. As an example, resource sdk doesn't use AttributeValue, but directly uses OwnedAttributeValue. The problem is with the implicit conversion of char * to bool in C++17, which seems to be corrected in C++20. I did some tests on std::variant, and it is actually storing string literal as a bool. Please see my comment on the issue you created.

[ThomsonTan]

Could we provide an explicit overload to SetResource with const char * to avoid potential future error?

[maxgolov]

I don't like that. I'm proposing to re-add const char * to variant. Just verified that it works, and including it in the Abseil Variant PR.

This re-add of const char * will give us much nicer user / developer experience, when we get to C++17 standard lib impl of variant.

[lalitb]

I don't like that. I'm proposing to re-add const char * to variant. Just verified that it works, and including it in the Abseil Variant PR.

This re-add of const char * will give us much nicer user / developer experience, when we get to C++17 standard lib impl of variant.

Just to understand, we are seeing this issue both with C++17 STL and absl::variant, and mpark::variant handles this nicely ?

[lalitb]

I don't like that. I'm proposing to re-add const char * to variant. Just verified that it works, and including it in the Abseil Variant PR.

This re-add of const char * will give us much nicer user / developer experience, when we get to C++17 standard lib impl of variant.

Sure closing this PR.