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

Change the implementation of ResourceAttributes to use common::AttributeValue #817

Closed
maxgolov opened this issue Jun 2, 2021 · 0 comments · Fixed by #771
Closed

Change the implementation of ResourceAttributes to use common::AttributeValue #817

maxgolov opened this issue Jun 2, 2021 · 0 comments · Fixed by #771
Assignees

Comments

@maxgolov
Copy link
Contributor

maxgolov commented Jun 2, 2021

Problem Statement

The issue is that presently ResourceAttributes uses common::OwnedAttributeValue here :

using ResourceAttributes =
    std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue>;

Now if we build with Abseil Variant or with C++17 std::variant ( #572 ), then it is impossible to properly initialize Resource Attribute with const char *. Because it gets converted to bool instead of being treated as string. In essence, it stores invalid value of the attribute. That is happening because OwnedAttributeValue variant does not define const char * as one of valid values. See the issue described here ( #733 ).

Proposed Solution

As discussed, we would like to avoid adding const char * to OwnedAttributeValue.. That implies now that ResourceAttributes should intake API common::AttributeValue instead in its constructor. And that class should have the const char *:

using AttributeValue =
    nostd::variant<bool,
                   int32_t,
                   int64_t,
                   uint32_t,
                   double,
                   const char *,
                   nostd::string_view,
                   nostd::span<const bool>,
                   nostd::span<const int32_t>,
                   nostd::span<const int64_t>,
                   nostd::span<const uint32_t>,
                   nostd::span<const double>,
                   nostd::span<const nostd::string_view>,
                   // Not currently supported by the specification, but reserved for future use.
                   // Added to provide support for all primitive C++ types.
                   uint64_t,
                   // Not currently supported by the specification, but reserved for future use.
                   // Added to provide support for all primitive C++ types.
                   nostd::span<const uint64_t>,
                   // Not currently supported by the specification, but reserved for future use.
                   // See https://github.com/open-telemetry/opentelemetry-specification/issues/780
                   nostd::span<const uint8_t>>;

Note common::AttributeValue allows const char *. Subsequently the collection of ResourceAttributes internally may still be backed by OwnedAttributeValue. We'd have to apply a transform of incoming ABI/API-safe collection - for each key-value from AttributeValue (that may contain const char *) to OwnedAttributeValue within the implementation of the Resource Attributes collection (that would reassign all non-owning const char * into owning std::string within).

Other Considerations / Future Work

This would also pave a way to potentially expose Resource Attributes either via API. And/or via ABI-stable interface - to be populated by instrumented application / library, by invoking SetResource / SetResources methods from vendor-provided prebuilt OpenTelemetry SDK DLL/Shared instrumentation library. This prebuilt library approach would only be applicable to later versions of SDK, when we get to solve various shared library integration questions in v1.1, i.e. already after v1.0.

@lalitb @pyohannes

@maxgolov maxgolov self-assigned this Jun 2, 2021
@maxgolov maxgolov linked a pull request Jun 2, 2021 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant