feat: instrument error reporting and tracing for go jobs#4819
Hidden character warning
feat: instrument error reporting and tracing for go jobs#4819michaelkedar merged 6 commits intogoogle:masterfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant step forward in observability for the Go jobs, introducing structured logging, error reporting, and distributed tracing. The implementation is largely well-executed, leveraging slog and OpenTelemetry effectively. I've identified a high-severity issue in the error reporting logic that would hinder error grouping in Google Cloud Error Reporting, and a couple of medium-severity inconsistencies in context initialization that should be addressed for better code uniformity. Once these points are addressed, this will be an excellent addition.
go/logger/init.go
Outdated
| ) | ||
|
|
||
| // InitGlobalLogger initializes the global slog logger and GCP observability clients. | ||
| func InitGlobalLogger(ctx context.Context) { |
There was a problem hiding this comment.
This is being ran with once, I don't think it makes sense to ask for context here, especially when all callers right now are using context.Background(). This might confused the caller to call this with their specific request context, which we don't want.
There was a problem hiding this comment.
Also, we can be fancy and use init() here, and it'll be automatically initialized by anyone that uses any logger functions. Removing the need to have all those if statements in each function. Or people forgetting to init the logger.
There was a problem hiding this comment.
I removed the context from InitGlobalLogger, and changed it slightly.
I don't want to chuck it in init, because I don't want to connect to GCP as a side effect of importing. Also, you're supposed to call/defer logger.Close() to flush any errors/traces, and I don't want that silently being forgotten.
I removed the lazy initialisation of the GCP logger - now if you log without calling InitGlobalLogger it will always use the local logger.
| res = resource.NewWithAttributes(semconv.SchemaURL, semconv.ServiceNameKey.String(serviceName)) | ||
| } | ||
|
|
||
| sampleRate := 0.05 |
There was a problem hiding this comment.
Please add a comment descrbing why this is 0.05,
This doesn't affect vulnfeeds (yet), since it uses a duplicate logging thing.
To instrument tracing, you have to propagate the context correctly, and the new
logger.[LEVEL]Context()commands.