-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore(logging): add instrumentation #6210
Conversation
Capture usage of the package per process by ingesting instrumentation log entry in each process that ingest at least one log. The ingestion of the instrumentation entry reflects the method used for ingesting first application log (sync/async). Fixes #6132
ingest intrumentation log entry with dedicated log ID
fix tests failing due to differences in runtime versions remove output test from example
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.
Ran out of time but here is some initial feedback to go off of. Will re-review in full after. Thanks!
keep happy path going left avoid named return values remove changes from apiv2 package reduce code footprint by using structs instead of pointers initialize "constant" values at start time
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 couple more suggestions, thanks for the previous updates!
logging/internal/instrumentation.go
Outdated
func SetIngestInstrumentation(f bool) bool { | ||
ok := false | ||
if f != ingestInstrumentation { | ||
mu.Lock() |
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.
Will this value ever change once set? If not should this just be a sync Once as well to avoid mutex contension? I think we should try to avoid putting a mutex on a potential hot-path for logging.
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.
Good catch, it will not. I will move to use *sync.Once
so I can manipulate the value in the tests.
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've made relatively large refactoring to address your suggestion. I also used this opportunity to separate "instrumentation" of the log entry being ingested from the main logic of the logging.
To support testing I had to keep the sync primitive in the internal
package but the rest of the code resides in logging
package but in the separate file.
LMK if you have further comments.
replace all context.TODO() with context.Background()
Replace sync.Mutex with sync.Once. Move code to logging package but leave sync artifact in internal to allow testing. Refactor testing to adapt the changes.
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 the code is looking great, just a couple of small things.
) | ||
|
||
const ( | ||
// ProdAddr is the production address. | ||
ProdAddr = "logging.googleapis.com:443" | ||
) | ||
|
||
// InstrumentOnce guards instrumenting logs one time | ||
var InstrumentOnce = new(sync.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.
Why is this declared in internal? Feels like it should live next to the code it intends to gaurd and/or be wrapped in a struct. Either this should move or the code that is being guarded should move here.
This can just be var InstrumentOnce sync.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.
The main reason was to enable tests in logging_test.go
to manipulate it. The problem is that unless it is reset it produces additional ingestion that can change the behavior of the tests. Since the tests in the logging_test.go
aren't in the same package (for this the logging_unexported_test.gois used) it is impossible to do when the sync primitive is in the
logging` package. Moving it to internal and making it accessible within the top level package enables the manipulations in tests.
I am open to ideas how it can be modified. I will be merging it "as is" for now, but wait for your response before merging it into the main.
Capture usage of the package per process by ingesting instrumentation log entry in each process that ingest at least one log entry. The instrumentation log entry will be ingested using the same Logger as the first log entry. If it will be ingested directly using Logging API, the instrumentation entry's Log ID will be set to `diagnostic-log`.
Capture usage of the package per process by ingesting instrumentation log entry in each process that ingest at least one log entry. The instrumentation log entry will be ingested using the same Logger as the first log entry. If it will be ingested directly using Logging API, the instrumentation entry's Log ID will be set to `diagnostic-log`.
Capture instrumentation info about the package by ingesting a single instrumentation log entry per process on the event the process sends an application log first time (i.e. calls Logger.LogSync or Logger.Log).
The instrumentation info capturing reflects the ingestion method:
When ingesting directly, the Log ID is set to
diagnostic-log
. When redirecting to standard output the entry will be ingested to the same Log ID as all other entries.Fixes #6132