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

Ability to use Standard Library instead of opentelemetry nostd classes #67

Closed
maxgolov opened this issue Apr 27, 2020 · 10 comments
Closed
Assignees
Labels
priority:p2 Issues that are not blocking release:required-for-ga To be resolved before GA release

Comments

@maxgolov
Copy link
Contributor

Due to ABI stability requirements OpenTelemetry C++ SDK provides the standard implementation of 'common' std-alike classes under nostd namespace:

  • function ref
  • shared_ptr
  • span
  • string_view
  • unique_ptr
  • variant

However, in some environments it may be of benefit to alias the nostd types to standard library, esp. if compiler has full support of C++17. And if the environment the SDK is being built for has no need to consume any external prebuilt exporters.

One way to achieve this is a custom compile-time build option (cmake flag) that aliases nostd with std namespace. Tests could be added to verify that the SDK still works well with recent version of Visual Studio, Apple llvm-clang, gcc, etc. (in essence, replacing the nostd implementations with std).

@g-easy
Copy link
Contributor

g-easy commented Apr 30, 2020

Due to ABI stability requirements OpenTelemetry C++ API provides nostd::*.

What would be required so that e.g. std::string_view could be transparently passed to API functions expecting a nostd::string_view?

@rnburn
Copy link
Contributor

rnburn commented Apr 30, 2020

@g-easy - that should be easy. Just add an implicit conversion constructor if the c++ standard is 17 or greater.

@rnburn
Copy link
Contributor

rnburn commented Apr 30, 2020

@maxgolov

One way to achieve this is a custom compile-time build option (cmake flag) that aliases nostd with std namespace.

I'd recommend a more granular approach, maybe using directives within the nostd namespace, e.g.

namespace nostd {
using std::string_view;
}

The nostd components were added in different versions of c++ so we'd want to alias only if there're available. For example std::span was added in C++20 and function_ref is a proposal that hasn't been added yet.

@maxgolov
Copy link
Contributor Author

maxgolov commented May 1, 2020

Thanks Ryan for your feedback. I'll take a look at that option. In most cases I'd expect vs2019 and latest clang to have most of that. function_ref can be kept as exceptional case until (if and when) becomes a standard.

@g-easy
Copy link
Contributor

g-easy commented May 4, 2020

IIUC Abseil went the using std::string_view; route and this caused ABI issues. I suspect we need a shim that translates std::string_view to nostd::string_view e.g. potentially swapping the pointer and the length fields.

@maxgolov
Copy link
Contributor Author

maxgolov commented May 30, 2020

@g-easy - I'll share some thoughts on this. I made it work with STL and Visual Studio 2017+ in my fork. ABI compat won't be an issue for customers statically linking both API and SDK in one. One hiccup I have is with nostd::span , which is non-standard yet. gsl::span implementation works equally well though:
https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/gsl-intro.md

maxgolov referenced this issue in maxgolov/opentelemetry-cpp May 30, 2020
- add stdtypes.h umbrella header
- move all "optional" headers to nostd/stl
- add optional dependency on GSL for gsl::span
- add build option to build API with/without Standard Library
- improvements to EventSender example
- Visual Studio project for EventSender
@maxgolov maxgolov self-assigned this Jun 1, 2020
@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 1, 2020

I'll send a proposed PR with changes later this week. I only tested this on Visual C++ 2019, working on tests for other platforms.

@0x4b
Copy link

0x4b commented Jun 24, 2020

@maxgolov has there been any progress on allowing STL? The interns contributing context API functionality are finding the nostd requirement to be more of a burden than we expected.

Having clarity around whether STL is banned just at the interface or also in the implementation would be a big help. What do you think about making note in CONTRIBUTING.md (I can send a PR)?

@reyang
Copy link
Member

reyang commented Jun 29, 2020

Related to #134.

@lalitb lalitb added priority:p2 Issues that are not blocking release:required-for-ga To be resolved before GA release labels Dec 16, 2020
@maxgolov
Copy link
Contributor Author

Main feature code is integrated in #374 . I'll add documentation and CI in a separate PR. Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Issues that are not blocking release:required-for-ga To be resolved before GA release
Projects
None yet
Development

No branches or pull requests

6 participants