[WIP] Ability to use Standard Library as a substitute for nostd:: classes #275

maxgolov · 2020-08-12T07:05:54Z

This is a summary of changes needed to swap the nostd classes with std:: analogs available in C++14/C++17/C++20.

Motivation

In certain scenarios it may be of benefit to compile the OpenTelemetry SDK from source using standard library container classes (std::map, std::string_view, std::span, std::variant) instead of OpenTelemetry nostd:: analogs (which were backported to C++11 and designed as ABI-stable). This PR also can be used as a foundation for alternate approach - to also allow bindings to abseil classes. That is in environments that would rather prefer Abseil classes over the standard lib or nostd.

The approach is to provide totally opaque (from SDK code / SDK developer perspective) mapping / aliasing from nostd:: classes back to their std:: counterparts, and that is only done in environments where this is possible. We continue fully supporting both models and run CI for both.

Pros and Cons:

using standard library implementation classes in customer code and on API surface: certain environments would prefer to have standard instead of non-standard due to security / performance / stability / debug'ability considerations.
smaller binary size: no need to marshal types from standard to nostd, then back to standard library. We use standard classes used elsewhere in the app.
better performance: no need to transform from 'native' standard library types, e.g. std::map via KeyValueIterable - we can bypass that transformation process when we know that ABI compat is not a requirement. ETA at least 1.5% to 3% better perf since a transform / memcpy is avoided.

Standard Library build flavor works best for statically linking the SDK in a process (environment where ABI compat is not a requirement), or for "header-only" implementation of SDK. We cannot employ this approach for shared libraries in environments where ABI compatibility is required:

SDK binary compiled with compiler A + STL B
may not be ABI compatible with the main executable compiled with compiler C + STL D on a same OS.
Thus, this approach works best for statically linking / header only, and may not work for providing 'prebuilt' SDK binary.

What's included in this proposed PR:

set of changes to build both flavors of SDK: nostd (OpenTelemetry backport of classes for C++11) vs stl (standard library), plus set of tests to validate both side-by-side.
set of changes to validate that all OpenTelemetry functionality is functional irrespective of what library you prefer.
aliasing from nostd:: to std:: namespace for compilers C++17 and above.
consistent handling of std::variant across various versions of Mac OS X. Backport of a view missing variant features, e.g. std::get and std::visit for older version of Mac OS X. Patches that enable proper handling of std::visit and std::variant irrespective of OS version to resolve this quirk.
ability to optionally borrow implementation of C++20 gsl::span from Microsoft Guidelines Support Library. This will no longer necessary for compilers that already natively support C++20.
set of tools to locally build the SDK with both options across various OS and compilers, including vs2017, vs2019, ubuntu-18.xx (C++17), ubuntu-20.xx (C++20), Mac OS X - in C++17 mode.
a few minor 'logistics` clean-up changes, e.g. a few compiler warning fixes.

…try-cpp

- 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

- Rename stdtypes.h to stltypes.h to avoid conflict with popular header name - Resolve an issue with backport of std::size - Create standalone example of event sender that statically links API+SDK using standard library types

… string literals.

- ETW (EventWriteString in XML format) - OutputDebugStringA (UTF-8) - File - plain k-v pairs - File - JSON Plus generic framework for adding new stream types and converters.

…span<uint8_t>

…mplementations

…as a build-time option

…try-cpp

codecov · 2020-08-12T07:10:54Z

Codecov Report

Merging #275 into master will increase coverage by 0.04%.
The diff coverage is 97.20%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   94.59%   94.64%   +0.04%     
==========================================
  Files         146      148       +2     
  Lines        6629     6754     +125     
==========================================
+ Hits         6271     6392     +121     
- Misses        358      362       +4

Impacted Files	Coverage Δ
api/include/opentelemetry/nostd/shared_ptr.h	`100.00% <ø> (ø)`
api/include/opentelemetry/nostd/span.h	`89.36% <ø> (+1.36%)`	⬆️
api/include/opentelemetry/nostd/string_view.h	`100.00% <ø> (+2.63%)`	⬆️
api/include/opentelemetry/nostd/unique_ptr.h	`100.00% <ø> (ø)`
api/include/opentelemetry/nostd/utility.h	`83.33% <ø> (ø)`
api/include/opentelemetry/nostd/variant.h	`96.68% <ø> (ø)`
api/include/opentelemetry/trace/noop.h	`85.18% <ø> (ø)`
api/include/opentelemetry/trace/span.h	`100.00% <ø> (ø)`
api/include/opentelemetry/trace/tracer.h	`100.00% <ø> (ø)`
api/test/metrics/noop_metrics_test.cc	`88.63% <ø> (ø)`
... and 48 more

…try-cpp

…try-cpp into master

- add HAVE_SPAN_BYTE for future byte array support

- add Visual Studio 2015 support - add absl::variant as alternative to nostd::variant

jsuereth

A bunch of general thoughts and questions.

I appreciate how much work went into this! High level comments:

can we make it explicit in tests/builds what build flags are being used?
Can we document a matrix of build flags we're going to support as well as what the "DEFAULT" release is, where we expect vendors to support ABI.
Is ABSEIL missing from the testing matrix for non MSVC++ compilers? Is it not needed?

Pardon me if I'm asking silly questions, still not 100% ramped up on code structure.

jsuereth · 2020-10-21T16:18:19Z

api/include/opentelemetry/common/attribute_value.h

                                      double,
                                      nostd::string_view,
+                                      const char *,
+#ifdef HAVE_SPAN_BYTE


Is this part of this PR or noise?

If so, are there any concerns around this #ifidef causing compatibility issues with dynamic linking SDKs, or are we ONLY using this in a static-linked manner?

@jsuereth - apologies, I'm gonna send another PR from a separate branch to minimize the 'noise'. My master branch contained changes that I needed for some other flows, e.g. byte array support is needed for my internal customer needs, which is logged as an open issue in OT spec (but not implemented yet).

I'll try to answer your questions in the new PR.

jsuereth · 2020-10-22T12:29:35Z

.gitmodules

+[submodule "third_party/ms-gsl"]
+	path = "third_party/ms-gsl"
+	url = https://github.com/microsoft/GSL
+	branch = master


should this be picking a specific version of ms-GSL for those who opt-in to it?

it wasn't clear from the repo how folks understand what version they're using and pull in bug fixes.

additionally, if we want to guarantee any kind of ABI/API compat we probably need to know exactly which version is getting linked in when the flag is used.

Right now it's pulling the latest. But since we use submodules - for CMake build we pin-point to specific commit ID. Technically the MS-GSL is only needed for older compilers without C++20 support, i.e. latest-latest Mac and latest gcc-9+ (as well as vs2019 with c++_latest flag) no longer require MS-GSL. As an alternative - I also want to add an option to build with ABSEIL instead of MS-GSL....

Re. pinning in Bazel - I am not sure how to do it. I wonder if we add Abseil, then test the combo of Bazel+Abseil, and CMake+GSL, that'd be covering the most needed build configurations for those who want to build statically. I can add CMake+Abseil build flavor.

jsuereth · 2020-10-22T12:37:57Z

CMakeLists.txt

+option(WITH_STL "Whether to use Standard Library for C++latest features" OFF)
+
+option(WITH_ABSEIL "Whether to use Abseil for C++latest features" OFF)
+


Nit: if both option are usable or not usable together, should we call that out?

@jsuereth - This is complicated, but I'll try to elaborate :)

technically Abseil would be best for vs2015 support, because there is no alternate good variant working with vs2015. Only absl::variant works. The one we borrowed from MPark - nostd::variant - does not compile for vs2015, to be more exact - it has problems with complex aggregate variant types.

now Abseil actually requires statically linked library of itself, which I 'hacked' - if we only use Abseil for variant and nothing else. Whereas GSL itself is header-only. In certain environments I'd prefer the full set of everything to be built with Standard Library and, preferably, header-only --- not needing Abseil (e.g. if I compile the OpenTelemetry SDK for inclusion in some Azure cloud Service).

So - it's problematic on all accounts everywhere:

nostd::variant - the one we have today, is not compiling on vs2015. The only one compiling is absl::variant

absl::variant - if used without any hacks, requires Abseil STATIC lib (messy)

whereas GSL only has gsl::span and does not have gsl::variant :(

So -- to answer your question, in my setup I would prefer:

just a portion of Abseil for absl::variant (but not the entire Abseil) + MS-GSL for span ... whereas

Google projects would probably prefer Abseil for everything ... whereas

dynamic libs prefer nostd:: classes... except this won't work for vs2015. We may need to borrow just the variant from Abseil, tweak it so that it doesn't require static linking of the core Abseil, and replace MPark (nostd::variant) with Abseil... Which I wanted to do in a separate PR.

jsuereth · 2020-10-22T12:38:48Z

api/CMakeLists.txt

  add_subdirectory(test)
 endif()
+
+if(WITH_STL)


Nit: can we include ABSEIL here too?

jsuereth · 2020-10-22T12:41:48Z

api/include/opentelemetry/context/runtime_context.h

+  static bool Detach(Token &token) noexcept { return ContextHandler()->InternalDetach(token); }

-  static RuntimeContext *context_handler_;
+  static inline IRuntimeContext *ContextHandler(IRuntimeContext *context_handler = nullptr)


is this new, and/or part of this PR (or just merge conflict overreporting?)

I'll clean it. Sorry, this is messy - I was showing an alternate implementation of context. I'll resend the PR from a clean branch.

jsuereth · 2020-10-22T13:03:32Z

examples/nostd/README.md

@@ -0,0 +1,63 @@
+# Usage example for nostd classes


nice writeup!

I'll send this in a separate PR.

jsuereth · 2020-10-22T13:04:43Z

exporters/otlp/BUILD

-        "@com_google_googletest//:gtest_main",
-    ],
-)
+# cc_test(


why is this disabled?

I'll reenable it.

jsuereth · 2020-10-22T13:05:42Z

exporters/otlp/src/recordable.cc

                       const opentelemetry::common::AttributeValue &value)
 {
+
+#if 0  // FIXME


Is this because of the HAVE_COLLECTIONS flag?

jsuereth · 2020-10-22T13:13:22Z

tools/build-vs2015-x64.cmd

+set "CMAKE_GEN=Visual Studio 14 2015 Win64"
+cd %~dp0
+call setup-buildtools.cmd
+call build.cmd -DWITH_ABSEIL:BOOL=ON


I wasn't sure what you meant originally around having multiple builds.

I thought you were implying we'd have a matrix of builds

Compiler x ABSEIL_FLAG x STD_FLAG

It looks like, instead, each version of VisualStudio gets a configuration. Do we need to make a visual matrix of that for users? While I definitely want the ability to statically link against STD we should make sure it's obvious which configurations support which and what you get out of the box.

Basically if we want to produce a static lib linkable with anything vs2015|vs2017|vs2019, then it MUST be built using vs2015. This is quite silly linker limitation. For example, if we statically produce a lib with vs2019, then it just won't link with older compilers!!! That's because the latest vs2019 has a bit different exception handling semantics.

I do want to have a matrix doc created.

jsuereth · 2020-10-22T13:14:28Z

tools/build.sh

+  make
+}
+
+build_configuration nostd '-DWITH_STL:BOOL=OFF'


do we also need ABSEIL ON/OFF permutations here?

I'll elaborate in the new PR.

chore(deps): update ubuntu:24.04 docker digest to 6015f66

maxgolov added 30 commits April 11, 2020 21:10

Fix permissions on shell scripts

5ea2e9f

Draft sketch of EventProperties variant for LogEvent API surface

96fa468

Remove unnecessary headers

9e398aa

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

1468119

…try-cpp

Add vcpkg submodule and build scripts to use it on Windows

9acb77c

Changes:

67a9f44

- Rename stdtypes.h to stltypes.h to avoid conflict with popular header name - Resolve an issue with backport of std::size - Create standalone example of event sender that statically links API+SDK using standard library types

Add OT types to event::Properties and event::Property

b003fc4

Add to_span_ptr template

10f499e

Add Close() method with default close duration

f3b9eae

Header-only implementation of Console Tracer

711de7a

Update EventSender example to show both API surfaces

f0427e5

Add enum for variant types. Add support for C-string const char * and…

4c1cd55

… string literals.

Transform from event::Properties to AttributeValue

cb7326a

Header-only Console Tracer implementation

8fe51b1

Add .gitignore for MSBuild project

f839760

Implementation of various tracer streams:

830a502

- ETW (EventWriteString in XML format) - OutputDebugStringA (UTF-8) - File - plain k-v pairs - File - JSON Plus generic framework for adding new stream types and converters.

Fix an issue with UUID std::string copy by converting UUID to nostd::…

1072ccf

…span<uint8_t>

Clean-up utils timestamp

9b185ba

Add Windows pipe example

7c8b159

Rename time field from ts to time

5d036c5

Rename to "StreamTracer" and comment out unused header for now

a222d52

Verified that standard STL with ms-gsl works with gcc-9 on Mac OS X

9ce018a

Fix an issue with clang

0e7c661

Fixes to ensure that iterators work properly for both nostd and STL i…

2557c35

…mplementations

Revert layout changes

b3a0c19

Trimmed down implementation of pure Standard Library STL API surface …

2c988c8

…as a build-time option

Add Google Test to submodules

1f718a2

Clean-up string_view iterators

4eeaaee

Remove googletest

3d30ff6

maxgolov added 9 commits August 11, 2020 18:29

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

d548ed4

…try-cpp

Update ms-gsl

cb8e437

Remove space

6f24b21

Update vcpkg

85fa56b

Print Bazel test errors in console

3a22787

Fix gcc build issue with std::size_t

ebd0727

Add cstring header for std::strlen

4cbdf9e

Revert reference change - it breaks on gcc-4.8

2fc1da0

Temporary patch to turn otlp exporter off

420867f

maxgolov requested a review from a team August 12, 2020 07:05

maxgolov added 4 commits August 12, 2020 08:54

Merge branch 'master' into master

611df39

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

fba85a4

…try-cpp

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

0407173

…try-cpp into master

- Update ETWTracer to be compatible with latest mainline changes

17dc5e6

- add HAVE_SPAN_BYTE for future byte array support

maxgolov mentioned this pull request Sep 3, 2020

context::ContextValue cannot be compiled with Visual Studio 2015 #314

Closed

maxgolov added 10 commits September 3, 2020 01:51

- refactor runtime_context.h

8dd6267

- add Visual Studio 2015 support - add absl::variant as alternative to nostd::variant

Remove threadlocal_context.h pollution

9761067

Add missing header to test/trace/noop_test.cc

3cb7423

Add ThreadLocalContext::stack_ to runtime_context_test.cc

90466a7

Add more details to ETW provider example

6ac7f0e

Fix formatting

2c8fbc5

Format code

34a7b0d

Add verbosity to format tool

1c725c4

Print list of files before actually crashing :)

4ce4e97

Big formatting change (no code changes) to make formatter happy

4004c2d

jsuereth reviewed Oct 22, 2020

View reviewed changes

maxgolov closed this Oct 26, 2020

reyang mentioned this pull request Nov 30, 2020

Add jsuereth as an Approver #409

Merged

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025

Merge pull request open-telemetry#275 from malkia/renovate/ubuntu-24.04

3c81eb6

chore(deps): update ubuntu:24.04 docker digest to 6015f66

		option(WITH_STL "Whether to use Standard Library for C++latest features" OFF)

		option(WITH_ABSEIL "Whether to use Abseil for C++latest features" OFF)

[WIP] Ability to use Standard Library as a substitute for nostd:: classes #275

[WIP] Ability to use Standard Library as a substitute for nostd:: classes #275

Uh oh!

Conversation

maxgolov commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jsuereth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxgolov commented Aug 12, 2020 •

edited

Loading

codecov bot commented Aug 12, 2020 •

edited

Loading