-
Notifications
You must be signed in to change notification settings - Fork 862
[api-logs] Expose logging artifacts as public #4552
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4552 +/- ##
==========================================
+ Coverage 85.16% 85.43% +0.26%
==========================================
Files 320 320
Lines 12677 12641 -36
==========================================
+ Hits 10797 10800 +3
+ Misses 1880 1841 -39
|
src/OpenTelemetry.Api/CHANGELOG.md
Outdated
|
|
||
| * Added [Logs Bridge | ||
| API](https://github.com/open-telemetry/opentelemetry-specification/blob/976432b74c565e8a84af3570e9b82cb95e1d844c/specification/logs/bridge-api.md) | ||
| artifacts. |
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.
What does "artifact" mean here? (which part of https://en.wikipedia.org/wiki/Artifact_(software_development))
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.
"Things" 🤣 I changed the entry to call out the main pieces LMK if you want it to say more.
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 guess many folks would interpret "adding ... artifacts" as "introducing a new NuGet package or DLL". I suggest that we simply say "added logs bridge API" ("to the existing API artifact" - which can be easily deduced from the changelog context).
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.
Something weird is going on with GitHub. I pushed to the branch behind this PR but the PR isn't updating. Here's the commit if you want to check out the text I'm trying to use 😄
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.
The new version is much easier to understand 👍
| @@ -1,0 +1,79 @@ | |||
| abstract OpenTelemetry.Logs.Logger.EmitLog(in OpenTelemetry.Logs.LogRecordData data, in OpenTelemetry.Logs.LogRecordAttributeList attributes = default(OpenTelemetry.Logs.LogRecordAttributeList)) -> void | |||
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.
Did we not try to avoid APIs with default parameters earlier?
I would have expected code analysis to complain about this. RS0026: Do not add multiple public overloads with optional parameters
Relates to #4433
Changes
OpenTelemetry.Apiand removes the addedInternalsVisibleTosMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes