-
Notifications
You must be signed in to change notification settings - Fork 438
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 Logging SDK implementation for Logging API #386
Conversation
Codecov Report
@@ Coverage Diff @@
## master #386 +/- ##
=======================================
Coverage 95.01% 95.01%
=======================================
Files 164 171 +7
Lines 7099 7186 +87
=======================================
+ Hits 6745 6828 +83
- Misses 354 358 +4
|
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.
Thanks a lot for all your great work on this. I have a few question/suggestions/opinions below.
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.
This looks like a great first PR for logging!
My high level comments:
- I saw TODOs about pulling LogRecord in via Heap. I'd also love to see support for pulling from a ring/mempool or some other mechanism to limit overall memory consumption from log-processing. We can discuss that in any follow-on PRs / SiG. (This is a nice-to-have)
- I have some reservation around the shared map + mutex currently. I saw the discussion and I think you're aware of the nuance you're in, I expect we'll want to throw some benchmarks around that code and tune it over time. If we find
GetLogger
is frequently used in user-code we'll want to make sure that's as efficient as it can be (e.g. components which instantiate on every incoming HTTP Request means we're calling it often, and on many threads). I think you should open a ticket around bench-marking and determining if we should try to migrate to RW lock. - Tactically, how are you tracking TODOs and follow on work? Is it feasible to link TODOs to issues on the project? Two reason for this:
- It'd be nice to understand what you plan to tackle yourself in follow-on PRs
- It'd be nice for others in the community to see whether or not a TODO is actively worked on or if they can pick it up.
We discussed the issue of propagating the |
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.
- There are several unnecessary methods from the Logger class, need to remove them (we can add them later if there is a need).
- The processor ownership seems incorrect, need to move it to the provider.
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.
This PR contains an initial implementation of the Logging SDK, which follows from the initial API commit and the logging prototype issue.
This PR adds:
Any suggestions or feedback would be greatly appreciated! The design doc for the Logging SDK will be released in a future PR.
cc @xukaren @alolita @reyang @ThomsonTan @maxgolov @tigrannajaryan