-
Notifications
You must be signed in to change notification settings - Fork 446
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
Mark logs signal as stable API/SDK #2229
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2229 +/- ##
==========================================
+ Coverage 87.32% 87.62% +0.31%
==========================================
Files 169 198 +29
Lines 4897 5856 +959
==========================================
+ Hits 4276 5131 +855
- Misses 621 725 +104
|
Per discussion in C++ SIG meeting (2023-07-12):
before merging this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
See a comment on empty_attributes.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thanks.
the current OpenTelemetry Log specification matures. The current | ||
implementation can be included in build by setting `ENABLE_LOGS_PREVIEW` | ||
preprocessor macro. | ||
| Logs | Public Release | N/A | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any plans to update the compliance matrix before marking this stable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will update it.
#include "opentelemetry/sdk/logs/logger_provider.h" | ||
#include "opentelemetry/sdk/logs/read_write_log_record.h" | ||
#include "opentelemetry/sdk/logs/simple_log_record_processor.h" | ||
#include "opentelemetry/sdk/version/version.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the above headers have been added twice, Can you please check this once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any header file is included twice directly. Do you mean some header file in the list is also included indirectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can fix this later if it is an issue.
Fixes #2172
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes