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

[SEMANTIC CONVENTIONS] Upgrade to version 1.15.0 #1761

Merged
merged 8 commits into from
Nov 16, 2022

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Nov 10, 2022

Fixes #1758

Changes

Upgrade semantic conventions to opentelemetry-specification 1.15.0
Change naming to use camel case, to avoid collisions with macros.

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

@marcalff marcalff requested a review from a team November 10, 2022 13:07
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #1761 (e1945b2) into main (a3e3a90) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1761   +/-   ##
=======================================
  Coverage   85.77%   85.77%           
=======================================
  Files         171      171           
  Lines        5214     5214           
=======================================
  Hits         4472     4472           
  Misses        742      742           
Impacted Files Coverage Δ
sdk/src/resource/resource.cc 96.30% <100.00%> (ø)

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.

Thanks for the upgrade.

build break on windows 2019 on ERROR
@marcalff
Copy link
Member Author

@lalitb

Please note: I had to implement a work around for ERROR in windows, similar to DELETE:

#ifdef _WIN32
#  ifdef ERROR
#    pragma message(__FILE__ ": removing define on ERROR")
#    undef ERROR
#  endif
#endif

Build on windows fails without it.

@ThomsonTan
Copy link
Contributor

@lalitb

Please note: I had to implement a work around for ERROR in windows, similar to DELETE:

#ifdef _WIN32
#  ifdef ERROR
#    pragma message(__FILE__ ": removing define on ERROR")
#    undef ERROR
#  endif
#endif

Build on windows fails without it.

Wondering why this is needed now.

@lalitb
Copy link
Member

lalitb commented Nov 11, 2022

@lalitb
Please note: I had to implement a work around for ERROR in windows, similar to DELETE:

#ifdef _WIN32
#  ifdef ERROR
#    pragma message(__FILE__ ": removing define on ERROR")
#    undef ERROR
#  endif
#endif

Build on windows fails without it.

Wondering why this is needed now.

I think this line is having conflict with ERROR macro

static constexpr const char *ERROR = "ERROR";

@ThomsonTan
Copy link
Contributor

@lalitb
Please note: I had to implement a work around for ERROR in windows, similar to DELETE:

#ifdef _WIN32
#  ifdef ERROR
#    pragma message(__FILE__ ": removing define on ERROR")
#    undef ERROR
#  endif
#endif

Build on windows fails without it.

Wondering why this is needed now.

I think this line is having conflict with ERROR macro

static constexpr const char *ERROR = "ERROR";

Can we rename the new variable here?

@lalitb
Copy link
Member

lalitb commented Nov 11, 2022

Can we rename the new variable here?

Yeah good point, if it is possible to do in the jinja template.

@marcalff
Copy link
Member Author

marcalff commented Nov 11, 2022

Some comments about ERROR:

  • I don't know what the ERROR macro is on Windows, and what it is for. So, no idea if undef ERROR is intrusive or not for the application code.
  • It is confirmed, from the build logs, that there is an ERROR macro somewhere, see bazel windows.
  • Could someone point out which header file on windows is defining an ERROR macro, will be needed in the comments.

If we rename it:

  • Any proposal about the name ?
  • Should we have some consistency about alternate names, like SEMCONV_<foo> to replace <foo> on name collisions ? It is likely that more collisions will happen again, with new semantic conventions.
  • Should the alternate name, say SEMCONV_ERROR, be used on the offending platform only or on all platforms ?
  • Should we change undef DELETE to SEMCONV_DELETE then ?

My preference at this point is:

  • change DELETE to SEMCONV_DELETE
  • change ERROR to SEMCONV_ERROR
  • apply for all platform, so user code that builds on multiple platforms uses the same symbol in all cases.
  • never undef anything, to avoid being intrusive for applications, even when they do not use the offending semantic convention (this is a change compared to previous solution for DELETE)

Requesting re-review to settle this.

@marcalff marcalff requested a review from lalitb November 11, 2022 11:32
@lalitb
Copy link
Member

lalitb commented Nov 11, 2022

@marcalff Should we instead change the variable name to follow the C++ style guide (https://google.github.io/styleguide/cppguide.html#Constant_Names) i.e, k<camelCase>. I see the to_camel_case method defined in templating code, not sure if we can use it and prefix k before it.

Also semantic conventions specs are still experimental, so we can use that to our benefit :)

marcalff and others added 4 commits November 11, 2022 23:41
This avoids any collision with macros, typically ALL_UPPER_CASE.
Added notes in CHANGELOG.
@marcalff
Copy link
Member Author

@marcalff Should we instead change the variable name to follow the C++ style guide (https://google.github.io/styleguide/cppguide.html#Constant_Names) i.e, k<camelCase>. I see the to_camel_case method defined in templating code, not sure if we can use it and prefix k before it.

Also semantic conventions specs are still experimental, so we can use that to our benefit :)

Thanks @lalitb, this is a much better solution.

Implemented.

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 :)

@lalitb lalitb merged commit d7a388b into open-telemetry:main Nov 16, 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_1758 branch July 4, 2023 07:18
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] Upgrade to version 1.15.0
3 participants