Skip to content

[SEMANTIC CONVENTIONS] Upgrade to version 1.15.0 - #1761

Merged
lalitb merged 8 commits into
open-telemetry:mainfrom
marcalff:fix_sem_conv_1758
Nov 16, 2022
Merged

lalitb merged 8 commits into
open-telemetry:mainfrom
marcalff:fix_sem_conv_1758

Conversation

@marcalff

@marcalff marcalff commented Nov 10, 2022 •

Copy link
Copy Markdown
Member

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

codecov Bot commented Nov 10, 2022 •

Copy link
Copy Markdown

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%> (ø)

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

lalitb commented Nov 11, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
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

lalitb commented Nov 11, 2022

Copy link
Copy Markdown
Member

Can we rename the new variable here?

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

@marcalff

marcalff commented Nov 11, 2022 •

Copy link
Copy Markdown
Member Author

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

lalitb commented Nov 11, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
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.

@lalitb lalitb left a comment •

Copy link
Copy Markdown
Member

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
@marcalff
marcalff deleted the fix_sem_conv_1758 branch July 4, 2023 07:18
@marcalff marcalff mentioned this pull request Nov 9, 2024
3 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 this pull request may close these issues.

[SEMANTIC CONVENTIONS] Upgrade to version 1.15.0

3 participants