feat: log opentelemetry logs and errors using a provided zap logger #239

varkey98 · 2024-10-07T15:06:35Z

Description

As of now, the opentelemetry logs and errors are printed to stdout. Adding logic to use a passed zap logger for logging these. Opentelemetry go uses go-logr API for internal logging and go-logr has a zap wrapper implementation which is used for passing the provided zap logger to the opentelemetry library.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2024-10-07T15:30:02Z

Codecov Report

Attention: Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 58.84%. Comparing base (4e0c822) to head (7739594).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
instrumentation/opentelemetry/init.go	72.72%	2 Missing and 1 partial ⚠️
...ntation/opentelemetry/batchspanprocessor/logger.go	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
- Coverage   60.40%   58.84%   -1.56%     
==========================================
  Files          55       72      +17     
  Lines        1836     2396     +560     
==========================================
+ Hits         1109     1410     +301     
- Misses        659      905     +246     
- Partials       68       81      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

varkey98 · 2024-10-07T15:30:53Z

error screenshot:

varkey98 · 2024-10-07T15:38:18Z

looking into static analysis failures

tim-mwangi · 2024-10-07T17:14:31Z

instrumentation/opentelemetry/errorhandler/handler.go

@@ -0,0 +1,22 @@
+package errorhandler


Can you check if this works for our custom batchspanprocessor or if it can be adapted the new zap logger? https://github.com/hypertrace/goagent/tree/main/instrumentation/opentelemetry/batchspanprocessor . We add some custom code in our batchspanprocessor like catching panics and metrics for spans received, sent and dropped. Since we cannot use the logger in the internal package in the otel go repo, we adapt it and even initialize a new stdout logger.

I didnt know we had a logger here. We can do it, zap has a global api. We can either wrap it in go-logr or use zap itself in the custom bsp

tim-mwangi · 2024-10-07T17:16:04Z

instrumentation/opentelemetry/init.go

@@ -245,6 +248,19 @@ func Init(cfg *config.AgentConfig) func() {
 // and returns a shutdown function to flush data immediately on a termination signal.
 func InitWithSpanProcessorWrapper(cfg *config.AgentConfig, wrapper SpanProcessorWrapper,
 	versionInfoAttrs []attribute.KeyValue) func() {
+	logger, err := zap.NewProduction()


Is there existing log config we can use to set some config in the zap logger we create?

We dont have any existing log config in the hypertrace proto. The traceable goagent initializes logger with this config: https://github.com/Traceableai/goagent-src/blob/main/init.go#L29. Maybe we can move that logic here? Or can just use the new zap compatible api there?

There's a techdebt ticket to move the hypertrace goagent login into traceable goagent and archive hypertrace goagent. So we should probably wait until we do that and in hypertrace we can go with some zap logger support. But it should be overridable by the traceable goagent one for now. I did not know that we already had a zap logger in traceable goagent.

Oh okay, then I think we should be good. We can pass that zap logger here with the new api

My ask here is that we should be able to use the logger created in the agent(i.e. mirroring-agent/ebpf) so that we can control where do the logs go(file/stdout/stderr), loglevels, rotation from one place.

Yes. As of now ebpf calls this api: https://github.com/Traceableai/ebpf/blob/main/pkg/export/exporter.go#L41
This calls the InitWithSpanProcessorWrapper here https://github.com/Traceableai/goagent-src/blob/main/init.go#L37. Now we'll update the goagent-src to also expose an api for taking in a zap logger, and will call InitWithSpanProcessorWrapperAndZap instead of InitWithSpanProcessorWrapper, then we can update the respective clients (ebpf/mirroring-agent/ext_cap)

Yeah. If the same zap logger or zapcore object can be used throughout, that would be ideal.

puneet-traceable · 2024-10-08T09:51:48Z

@varkey98 Trying to understand how would batchSpanProcessor get the zap.Logger instance which is created for ext_cap/mirroring-agent/ebpf?

varkey98 · 2024-10-08T15:45:03Z

@varkey98 Trying to understand how would batchSpanProcessor get the zap.Logger instance which is created for ext_cap/mirroring-agent/ebpf?

It'll be like the goagent client calls InitWithSpanProcessorWrapperAndZap which will set a global zap logger. Then bsp on initialization will call it's logger package which was initialized to std out logger earlier, now it'll be initialized to the zap's global logger

varkey98 · 2024-10-08T15:47:06Z

One caveat is that if any of our agents set a zap global logger, that will be overwritten during goagent initialization. I dont think any of our agents do this, but we should remember this in the future

puneet-traceable · 2024-10-09T11:59:56Z

instrumentation/opentelemetry/batchspanprocessor/logger.go

+// since global variables init happens on package imports,
+// the zap global logger might not be set by then
+// hence doing a lazy initialization
+func getLogger() logr.Logger {


Should we instead pass logger here

goagent/instrumentation/opentelemetry/init.go

Line 309 in 94709da

sp := modbsp.CreateBatchSpanProcessor(

and store that in batch_span process struct? What I am unsure is what this global logger is? Secondly if intend to pass a sampling logger from the agent, how would that work? Does it apply rotation that's configured?

This works with whatever logger we pass as of now. I've checked with file logger on TPA. Because we set the global logger here: https://github.com/hypertrace/goagent/blob/main/instrumentation/opentelemetry/init.go#L288. Only case this wont work is when someone sets another global zap logger from another go routine. But I dont think we do that anywhere. I didnt want to set it in the bsp struct because that'll modify the function interface compared to otel and thought we didnt want to do that. @tim-mwangi ?

traceable goagent has it's own global logger implemented with zap, I thought that when we port this to that repo, we can change the bsp to our global logger instead of zap global logger and this as makeshift mechanism for the time being

The reason for the logger being adapted is that it is an internal package and so cannot be used in our customized batchspanprocessor. So it the env. https://github.com/open-telemetry/opentelemetry-go/blob/64416533d5cc340a8cc53d98084604758283af09/sdk/trace/batch_span_processor.go#L13

"go.opentelemetry.io/otel/internal/global" "go.opentelemetry.io/otel/sdk/internal/env"

So we can pass it but be careful to maintain the custom changes.

Should be okay for now. I did not see that replaceLogger call earlier.

puneet-traceable · 2024-10-09T12:03:01Z

instrumentation/opentelemetry/batchspanprocessor/logger.go

 }

 // Warn prints messages about warnings in the API or SDK.
 // Not an error but is likely more important than an informational event.
 func Warn(msg string, keysAndValues ...interface{}) {
-	logger.V(1).Info(msg, keysAndValues...)
+	getLogger().V(1).Info(msg, keysAndValues...)


Also, do we need separate verbosity levels here? How does that gets configured?

Not sure about this, kept the existing behavior. cc: @tim-mwangi

This is based on the adaptation from https://github.com/open-telemetry/opentelemetry-go/blob/main/internal/global/internal_logging.go. I believe they are the levels in the logr package. If we are somehow hooking zap under the logr package, I think they should be compatible?

test added 5 commits June 23, 2024 17:03

updating span interface to have a remove event function

aaed7ca

using zap logger

fe5a531

go tidy changes

dace63c

fix lint

ccafcde

fix go mod

41267f8

varkey98 requested review from tim-mwangi, prodion23, ryanericson and puneet-traceable October 7, 2024 15:30

varkey98 self-assigned this Oct 7, 2024

PrajwalKrishna approved these changes Oct 7, 2024

View reviewed changes

tim-mwangi reviewed Oct 7, 2024

View reviewed changes

test added 2 commits October 8, 2024 00:21

using logger for bsp

6002521

osrt imports

7739594

tim-mwangi approved these changes Oct 8, 2024

View reviewed changes

varkey98 merged commit 94709da into hypertrace:main Oct 8, 2024
3 of 6 checks passed

varkey98 deleted the logger branch October 8, 2024 19:07

puneet-traceable reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: log opentelemetry logs and errors using a provided zap logger #239

feat: log opentelemetry logs and errors using a provided zap logger #239

varkey98 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

varkey98 commented Oct 7, 2024

varkey98 commented Oct 7, 2024

tim-mwangi Oct 7, 2024

varkey98 Oct 7, 2024

varkey98 Oct 7, 2024

tim-mwangi Oct 7, 2024

varkey98 Oct 7, 2024

tim-mwangi Oct 8, 2024

varkey98 Oct 8, 2024

puneet-traceable Oct 8, 2024

varkey98 Oct 8, 2024 •

edited

Loading

tim-mwangi Oct 9, 2024

puneet-traceable commented Oct 8, 2024

varkey98 commented Oct 8, 2024

varkey98 commented Oct 8, 2024

puneet-traceable Oct 9, 2024

varkey98 Oct 9, 2024 •

edited

Loading

varkey98 Oct 9, 2024

tim-mwangi Oct 9, 2024

puneet-traceable Oct 9, 2024

puneet-traceable Oct 9, 2024

varkey98 Oct 9, 2024

tim-mwangi Oct 9, 2024

feat: log opentelemetry logs and errors using a provided zap logger #239

feat: log opentelemetry logs and errors using a provided zap logger #239

Conversation

varkey98 commented Oct 7, 2024 • edited Loading

Description

Testing

Checklist:

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

varkey98 commented Oct 7, 2024

varkey98 commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkey98 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puneet-traceable commented Oct 8, 2024

varkey98 commented Oct 8, 2024

varkey98 commented Oct 8, 2024

Choose a reason for hiding this comment

varkey98 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkey98 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

varkey98 Oct 8, 2024 •

edited

Loading

varkey98 Oct 9, 2024 •

edited

Loading