Adding semantic-convention attributes for trace by lalitb · Pull Request #868 · open-telemetry/opentelemetry-cpp

lalitb · 2021-06-21T11:16:03Z

Fixes #566

Changes

This PR adds semantics as defined here:
https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/trace/semantic_conventions

This will let us use uniform attributes/events/exceptions names across services.

This is currently used in the HTTP and gRPC example. These may be used in exporters too, would create PR to replace there.

For significant contributions please make sure you have completed the following items:

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

codecov · 2021-06-21T11:28:32Z

Codecov Report

Merging #868 (3818109) into main (ef16d00) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #868   +/-   ##
=======================================
  Coverage   95.36%   95.36%           
=======================================
  Files         159      159           
  Lines        6776     6776           
=======================================
  Hits         6461     6461           
  Misses        315      315

Impacted Files	Coverage Δ
.../opentelemetry/sdk/resource/semantic_conventions.h	`100.00% <ø> (ø)`

maxgolov · 2021-06-21T16:50:58Z

+#include "opentelemetry/version.h"
+
+#define OTEL_TRACE_GENERATE_SEMCONV_METHOD(attribute_name, attribute_value) \
+  static const nostd::string_view Get##attribute_name() noexcept            \


Ideally this should be expressible as constexpr - to return the value of it at compile time, without incurring the overhead of a static method call. I'm not sure if the getters give significant benefit, as in - better structure or something? From Google C++ style document: Global strings: if you require a global or static string constant, consider using a simple character array, or a char pointer to the first element of a string literal. String literals have static storage duration already and are usually sufficient.

How can we do it in an all header way without getters? C++11 won't allow initializing static std::string/nostd::string within class definition.

You just don't use getters. Use constants with constexpr definition?

Or if you use getters, return straight the literal constant type without any class:

Or if you use getters, return straight the literal constant type without any class:

Thanks for your suggestions. I have modified the code to create the constrxpr function and return literal. For another suggestion on using the constrxpr variable, somehow, I don't see these getting optimized in different translation units by gcc compiler ( small pingy code to demonstrate that: https://www.onlinegdb.com/IjRRjLjZo ). But anyway preferred the constexpr function as we don't need to depend on linker/compiler optimization here.

To summarise the available options:
option1: constexpr static class methods: The advantage of using these is that only those methods are generated (in .text segment) at compile time which are used across all the translation units. The rest of the unused methods are ignored.
option2: constexpr static global functions in the namespace: This will create a copy of the used functions in each translation unit while ignoring the rest of the unused functions.
option3: constexpt static variables in the namespace: This will create a local copy of all the variables in each of the translation units including the header, and it seems that the gcc compiler doesn't optimize it.

The right solution comes from the C++17 inline variables, but option1 seems to be the correct approach for C++11 which is now implemented in this PR. We can discuss if there are any issues with the approach.

maxgolov · 2021-06-21T16:51:42Z

-                options);
+    auto span = get_tracer("http-server")
+                    ->StartSpan(span_name,
+                                {{SemanticConventions::GetAttributeHttpServerName(), server_name},


It looks rather bulky, in my opinion.

Agree. Do we have a better way of handling this?

There are several options:

make methods constexpr and return a string literal constant, inited as const char * or char array

or just define all string constants in a dedicated namespace, that enables using for those who want to write it in a shorter form. Then for gcc / clang this scenario is already optimized, and for msvc we need to enable string pooling to combine all strings into one region of memory. These constants all have global static duration, there is no need to "get" them, they'd be efficiently compile-time optimized. Using string_view for that is an overkill.

Thanks for the suggestion. I did initially wanted to use the second option but wasn't aware of the compiler optimization of this static global across multiple translation units. Let me try that.

As mentioned in prev comment, have created the constexpr function now.

ThomsonTan · 2021-06-21T17:22:08Z

+  // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md
+
+  // Genetal network connection attributes
+  OTEL_TRACE_GENERATE_SEMCONV_METHOD(AttributeNetTransport, "net.transport")


Expand Attribute in name AttributeNetTransport in the macro of OTEL_TRACE_GENERATE_SEMCONV_METHOD instead of duplicating it for every macro call?

Good point. Will do that.

This is done now.

ThomsonTan · 2021-06-21T17:38:42Z

+
+  // Genetal network connection attributes
+  OTEL_TRACE_GENERATE_SEMCONV_METHOD(AttributeNetTransport, "net.transport")
+  OTEL_TRACE_GENERATE_SEMCONV_METHOD(AttributeNetPeerIp, "net.peer.ip")


Could we generate these definitions from the YAML definition of semantic conventions such as https://github.com/open-telemetry/opentelemetry-specification/blob/main/semantic_conventions/trace/http.yaml?

...and could we generate them, and make them constexpr string constants? These would be encouraged by Google C++ coding style, and provide the most optimal compile-time optimization. Global strings: if you require a global or static string constant, consider using a simple character array, or a char pointer to the first element of a string literal. String literals have static storage duration already and are usually sufficient. .. constexpr string literal or string array should work, with all of these constants having unique prefix. Rather than grouping them in class - we group them in namespace. There is also a compile time flag that allows string pooling, we use that to avoid bloating in case if the same header gets included in different compilation units. For Visual Studio compiler that is /GF (Eliminate Duplicate Strings). And for gcc / clang I think it's on by default?

Could we generate these definitions from the YAML definition of semantic conventions such as

I did think of writing a script to parse the yaml file and generate C++ code containing these consts from that and add it as part of CI. But this data is not going to change on daily basis, and instead, it would be fine to modify our code a couple of times before GA to be consistent.
For otel-python and otel-go, they do have a script to generate code for these constants, along with other semantic data ( eg creating enums for allowable values for attributes, defining constraints of the list of attributes to be used etc) which we don't need to do. The current approach in this PR is as done by otel-dotnet.

@ThomsonTan - Have created issue for automatic generation of these definitions : #873
Once we finalize the approach to use, first ( and probably initial few ) definitions can be committed manually while someone works on a script to generate code as per the finalized approach. Hope that works.

Co-authored-by: Tom Tan <lilotom@gmail.com>

lalitb · 2021-06-24T09:19:29Z

@maxgolov @ThomsonTan I have modified the current implementation in this PR as per the logic used for sem-conv for Resources in #872. Please review it again. Thanks.,

lalitb · 2021-07-20T15:49:58Z

@open-telemetry/cpp-approvers. @maxgolov - Need help reviewing it. Changes are similar to what we implemented for already merged PR for resources: #872. Thanks.

sem conv for trace

782aa61

lalitb requested a review from a team June 21, 2021 11:16

new line at end

2067a24

maxgolov reviewed Jun 21, 2021

View reviewed changes

ThomsonTan reviewed Jun 21, 2021

View reviewed changes

Comment thread api/include/opentelemetry/trace/semantic_conventions.h Outdated

ThomsonTan reviewed Jun 21, 2021

View reviewed changes

lalitb and others added 2 commits June 21, 2021 23:59

Update api/include/opentelemetry/trace/semantic_conventions.h

f947863

Co-authored-by: Tom Tan <lilotom@gmail.com>

use constexpr instead of const

927447a

lalitb mentioned this pull request Jun 22, 2021

Adding semantic-convention attributes for resources #872

Merged

3 tasks

Merge branch 'main' into sem-conv-trace

d3da90b

lalitb mentioned this pull request Jun 23, 2021

Semantic Conventions - script to automatic generation of semantic conventions c++ code #873

Closed

lalitb and others added 5 commits June 24, 2021 10:46

Merge branch 'main' into sem-conv-trace

3a4d6b1

reimplement

035a0af

fix grpc example

2c88f16

fix grpc

b440636

final fix for grpc

24cc6b3

Merge branch 'main' into sem-conv-trace

9db4611

ThomsonTan approved these changes Jul 30, 2021

View reviewed changes

Merge branch 'main' into sem-conv-trace

3818109

lalitb merged commit 5414ebe into open-telemetry:main Jul 30, 2021

Conversation

lalitb commented Jun 21, 2021

Changes

Uh oh!

codecov Bot commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

lalitb Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb commented Jun 24, 2021

Uh oh!

lalitb commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jun 21, 2021 •

edited

Loading

lalitb Jun 21, 2021 •

edited

Loading

lalitb Jun 23, 2021 •

edited

Loading