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 note on experimental semantic convention implementation, prefix semantics headers with experimental tag #970

Merged
merged 11 commits into from
Sep 13, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Sep 3, 2021

Fixes 968

Changes

  • Add comment for semantic convention for resource and trace - to indicate it's based on experimental specs, and may change in future.
  • Rename experimental headers with experimenta_* prefix
  • Rename few src files having .cpp extension to .cc to be consistent with other src files.

cc @jsuereth
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

@lalitb lalitb requested a review from a team September 3, 2021 04:39
@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #970 (2aa8755) into main (c8b35ff) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #970   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files         161      161           
  Lines        6817     6817           
=======================================
  Hits         6502     6502           
  Misses        315      315           
Impacted Files Coverage Δ
...y/sdk/resource/experimental_semantic_conventions.h 100.00% <ø> (ø)
sdk/src/resource/resource.cc 92.00% <ø> (ø)
sdk/test/resource/resource_test.cc 93.46% <ø> (ø)

@ThomsonTan
Copy link
Contributor

@jsuereth do you think this declaration for semantic convention is sufficient, or we also need to declare the status in the release document?

@lalitb
Copy link
Member Author

lalitb commented Sep 3, 2021

@jsuereth do you think this declaration for semantic convention is sufficient, or we also need to declare the status in the release document?

Good point. We should be adding it in release description for 1.0.0 as done by other SIGs :
Eg Java https://github.com/open-telemetry/opentelemetry-java/releases/tag/v1.5.0

@@ -65,6 +65,12 @@ Refer to the [ABI Policy](./docs/abi-policy.md) for more details. To summarise:
allowed to break existing stable interfaces. Feature flags will be removed
once we have a stable implementation for the signal.

* As an exception, small experimental features in otherwise stable signals/components
mayn't necessarily be released under feature flag. These would be flagged as experimental
by adding a `NOTE` in it's header file - either at the beginning of file, or as the comment for
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking comment -

Would it make sense to call the header file experimental_* to further clarify?

I think you could do this and still provide backwards compatibility later by forward-chain including the new non-experimental folder when it stabilizes. Just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this does makes sense. Will incorporate this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done now.

@ThomsonTan ThomsonTan merged commit 320c593 into open-telemetry:main Sep 13, 2021
@lalitb lalitb changed the title Add note on experimental semantic convention implementation Add note on experimental semantic convention implementation, prefix semantics headers with experimental tag Sep 16, 2021
@lalitb lalitb mentioned this pull request Sep 17, 2021
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.

3 participants