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

Add automatically semantic conventions from the spec (#873) #1497

Merged
merged 15 commits into from
Jul 21, 2022

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jul 14, 2022

Fixes #873

Changes

Generate automatically semantic conventions from the opentelemetry specs.

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
  • Added documentation

@marcalff marcalff requested a review from a team July 14, 2022 23:39
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #1497 (1d508ba) into main (beba414) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1497      +/-   ##
==========================================
- Coverage   84.72%   84.71%   -0.01%     
==========================================
  Files         156      155       -1     
  Lines        4784     4778       -6     
==========================================
- Hits         4053     4047       -6     
  Misses        731      731              
Impacted Files Coverage Δ
sdk/src/resource/resource.cc 96.30% <100.00%> (-0.47%) ⬇️
...y/sdk/resource/experimental_semantic_conventions.h

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for picking this, it was long pending item :)

It would be good to document this script execution somewhere. One of the options could be in RELEASING.md if we want to recommend generating semantic conventions before every release. Or anywhere is fine as long as it is documented.

@marcalff marcalff changed the title [RFC] Automatically generate semantic conventions from the spec (#873) Automatically generate semantic conventions from the spec (#873) Jul 15, 2022
@marcalff
Copy link
Member Author

Do not merge yet.

Pushing temporary debug code to investigate windows build failure.

diff --git a/examples/http/server.cc b/examples/http/server.cc
index cf7e6bb..4687d1c 100644
--- a/examples/http/server.cc
+++ b/examples/http/server.cc
@@ -3,6 +3,13 @@

#include "server.h"
#include "opentelemetry/trace/context.h"
+
+// DEBUG: investigating build break
+
+#ifdef DELETE
+#error "Someone broke semantic_conventions"
+#endif
+
#include "opentelemetry/trace/semantic_conventions.h"
#include "tracer_common.h"

…etry#873)

Fixed markdown format.

Added code to troubleshoot build failure.
@marcalff
Copy link
Member Author

LGTM. Thanks for picking this, it was long pending item :)

It would be good to document this script execution somewhere. One of the options could be in RELEASING.md if we want to recommend generating semantic conventions before every release. Or anywhere is fine as long as it is documented.

Thanks @lalitb

Added documentation.

There is a very weird build break on windows,
I suspect some define pollution.

Trying to investigate that with CI, and might need some help (I don't have a windows os).

@marcalff
Copy link
Member Author

Do not merge yet.

Resolved and ok to merge.

On windows, some header file causes to define the symbol DELETE,
which conflicts with semantic convention FaasDocumentOperationValues::DELETE.

Implemented a work around, and CI on windows now passes.

…etry#873)

Removed #warning, not supported by the windows compiler.
examples/http/client.cc Outdated Show resolved Hide resolved
@marcalff marcalff changed the title Automatically generate semantic conventions from the spec (#873) Add automatically semantic conventions from the spec (#873) Jul 21, 2022
@marcalff
Copy link
Member Author

marcalff commented Jul 21, 2022

Revisited the windows work around for DELETE.

Now that the root cause is well identified, and affects one platform only,
implementing the work around -- for that platform -- in the template file is (more) acceptable:

  • this is the best solution, for ease of use. Many users are likely to experience build breaks without the work around because winnt.h is hard to avoid for people using this platform, so we might as well provide it for everyone using windows in the generated file semantic_conventions.h
  • the examples are now clean of any work around

Also, the generate script now discloses which semantic convention is compiled, with:

  • -Dsemconv=trace
  • -Dsemconv=resource

This will make implementing work around for other issues, should that be necessary, easier.

@ThomsonTan Thanks for finding the offending header.

@lalitb Last item about the DELETE work around now resolved (in a better way, in the template)

Ready to merge.

@lalitb
Copy link
Member

lalitb commented Jul 21, 2022

Thanks @marcalff , @ThomsonTan . Merging it.

@lalitb lalitb merged commit 0948054 into open-telemetry:main Jul 21, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
@marcalff marcalff deleted the fix_sem_conv_873 branch July 4, 2023 07:12
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 this pull request may close these issues.

Semantic Conventions - script to automatic generation of semantic conventions c++ code
4 participants