[accountingservice] adds otel logging support #1477

Kimbohlovette · 2024-03-23T13:01:27Z

The PR implements logging in the accounting service with slog in order to the slog.LogAttrs() which always receives the current context and attributes.
Use the otelslog - currently in development to collect logs and send to open telemetry.

…ntingservice-1/7

Kimbohlovette · 2024-03-23T13:03:09Z

@puckpuck @jparsana please take a look at this PR

…g log.WithContext()

julianocosta89

Hello @Kimbohlovette, thanks for taking care of this.

I've tested the PR but I can't see any logs being exported from accountingservice via OTLP.

From the OTel Go repo, I can see that this is still in development.

@pellared do we already have a sample/test for Go OTLP logs?

Kimbohlovette · 2024-03-25T12:47:34Z

Alright @julianocosta89. I focused on reproducing the same behavior using otellogrus
Any suggestions on what I could do to get the expected results? I would add that immediately

pellared

This does NOT use any bridge using OTel Go Logs Bridge API (experimental) nor OTel Go Logs SDK (under-development).

Kimbohlovette · 2024-03-25T14:01:51Z

@pellared is there some recommendation on what I should do to implement what we want?

austinlparker

This PR is using https://github.com/uptrace/opentelemetry-go-extra, not OpenTelemetry Logging. I'm not sure what the state of the upstream logging bridges are, but I don't think this actually 'adds logging support' at all, rather it's using the logrus utility from the linked repo to attach logs as span events.

pellared · 2024-03-25T14:32:04Z

@pellared is there some recommendation on what I should do to implement what we want?

Use slog for logging. Use https://pkg.go.dev/log/slog#Logger.LogAttrs for emitting log record to not lose the tracing context and to add attributes efficiently.
Use OTel slog bridge: https://pkg.go.dev/go.opentelemetry.io/contrib/bridges/otelslog (it is under-development. more: Add slog log bridge opentelemetry-go-contrib#5138)
Setup OTel Logs SDK: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log (it is under-development. more: https://github.com/orgs/open-telemetry/projects/43)

Kimbohlovette · 2024-03-25T14:36:57Z

@pellared is there some recommendation on what I should do to implement what we want?

Use slog for logging. Use https://pkg.go.dev/log/slog#Logger.LogAttrs for emitting log record to not lose the tracing context and to add attributes efficiently.

Use OTel slog bridge: https://pkg.go.dev/go.opentelemetry.io/contrib/bridges/otelslog (it is under-development. more: Add slog log bridge opentelemetry-go-contrib#5138)

Setup OTel Logs SDK: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log (it is under-development. more: https://github.com/orgs/open-telemetry/projects/43)

Okay @pellared many thanks, I will try to implement it that way. If I get any difficulties I will ping you guys immediately

pellared · 2024-03-28T07:06:09Z

Can you please convert this PR to a draft until it is ready to review?

Kimbohlovette · 2024-03-28T07:52:17Z

Can you please convert this PR to a draft until it is ready to review?

Okay

Kimbohlovette · 2024-03-28T09:01:51Z

Hello @pellared I just replaced logrus with slog and currently using slog.LogAttrs() & slog.Log() to emit records to os.Stderr. About using the Otel Log bridge, that is , the otelslog, I can't figure out how to add it the the service since it currently does not have examples in its docs. I could you unblock me here please?

pellared · 2024-03-28T09:15:18Z

As mentioned, it is under-development. You need to wait.

Kimbohlovette · 2024-03-28T09:25:21Z

As mentioned, it is under-development. You need to wait.

Okay thanks

pellared

I made a quick review for you.

pellared · 2024-03-28T09:53:38Z

src/accountingservice/main.go

-	}
-	log.Out = os.Stdout
+func initLogger() *slog.Logger {
+	logger := slog.New(slog.NewJSONHandler(os.Stderr, nil)).With("service", "accounting")


This is the place where you would use to otelslog bridge instead of slog.NewJSONHandle.

pellared · 2024-03-28T09:54:00Z

src/accountingservice/main.go

 	tp, err := initTracerProvider()
 	if err != nil {
-		log.Fatal(err)
+		logger.LogAttrs(ctx, slog.LevelError, "failed to initialize trace provider", slog.String("error", err.Error()))


Suggested change

logger.LogAttrs(ctx, slog.LevelError, "failed to initialize trace provider", slog.String("error", err.Error()))

logger.LogAttrs(ctx, slog.LevelError, "Failed to initialize trace provider", slog.String("error", err.Error()))

pellared · 2024-03-28T09:55:01Z

src/accountingservice/main.go

-		if err := tp.Shutdown(context.Background()); err != nil {
-			log.Printf("Error shutting down tracer provider: %v", err)
+		if err := tp.Shutdown(ctx); err != nil {
+			logger.LogAttrs(ctx, slog.LevelError, "failed to shotdown properly", slog.String("error", err.Error()))


Suggested change

logger.LogAttrs(ctx, slog.LevelError, "failed to shotdown properly", slog.String("error", err.Error()))

logger.LogAttrs(ctx, slog.LevelError, "Failed to shutdown properly", slog.String("error", err.Error()))

pellared · 2024-03-28T09:56:15Z

src/accountingservice/main.go

 	}()

 	<-ctx.Done()

-	log.Println("Accounting service exited")
+	logger.Log(ctx, slog.LevelInfo, "message", "Accounting service exited")


Suggested change

logger.Log(ctx, slog.LevelInfo, "message", "Accounting service exited")

logger.LogAttrs(ctx, slog.LevelInfo, "Accounting service exited")

pellared · 2024-03-28T09:57:13Z

src/accountingservice/main.go

 		}
-		log.Println("Shutdown trace provider")
+		logger.LogAttrs(ctx, slog.LevelInfo, "", slog.String("message", "Shotdown trace provider"))


Suggested change

logger.LogAttrs(ctx, slog.LevelInfo, "", slog.String("message", "Shotdown trace provider"))

logger.LogAttrs(ctx, slog.LevelInfo, "Shutdown trace provider")

pellared · 2024-03-28T09:57:21Z

src/accountingservice/main.go

-	log.Out = os.Stdout
+func initLogger() *slog.Logger {
+	logger := slog.New(slog.NewJSONHandler(os.Stderr, nil)).With("service", "accounting")
+	slog.SetDefault(logger)


Why are you setting the default logger. You never use it afterwards.

Consider removing this line or not returning the logger in this function and using slog.LogAttrs instead of logger.LogAttrs.

Okay I will do the changes as requested
Thank you for the reviews 😊

Kimbohlovette · 2024-03-28T10:03:43Z

I made a quick review for you.

Okay 😊

…ger()

Kimbohlovette · 2024-03-28T10:54:49Z

I have mentained the initialization logger := slog.New(.. at the top of main() because it is passed to the kafka.StartConsumerGroup() method

pellared · 2024-04-02T10:37:16Z

src/accountingservice/kafka/consumer.go

-				"messageTimestamp": message.Timestamp,
-				"messageTopic":     message.Topic,
-			}).Info("Message claimed")
+			g.log.LogAttrs(session.Context(), slog.LevelInfo, "Message claimed", slog.String("orderId", orderResult.OrderId), slog.String("messageTimestamp", message.Timestamp.String()), slog.String("messageTopic", message.Topic))


Notice that session.Context() would NOT contain the trace context information as NewOTelInterceptor does not pass the context created in oi.tracer.Start. Therefore, the logs would not contain the TraceID and SpanID fields.

Okay, so should I just use a new context like context.TODO()?

You should try to refine the sarama instrumentation so that you can propagate trace context to the logs.

julianocosta89 · 2024-04-04T10:03:05Z

I spoke with @Kimbohlovette today and mentioned that this is not supported yet as OTel logs in Go are still in development.
He will reach out to the OTel Go team to check if he can help out on bringing this to Stable.

Closing this PR for now.

pellared · 2024-04-04T11:01:11Z

@julianocosta89, @Kimbohlovette can still find out how to address #1477 (comment). Even though that OTel Go Logs are in development it still something that can be worked on. I cannot find anything more useful to help in OTel Go Logs. We will probably have a usable experimental release of OTel Go Logs this or next month.

It is already possible to use Logs Bridge API and slog Bridge which is everything needed for addressing #1477 (comment).

julianocosta89 · 2024-04-04T11:14:17Z

@pellared if we can make it work, than that would be great!
Unfortunately I'm a bit out of time to guide @Kimbohlovette on this. Would you know someone, or would you be able to help him out?

Once we get one working, he can apply the same solution for the other 2 Go services.

pellared · 2024-04-04T11:24:54Z

Would you know someone, or would you be able to help him out?

I already do my best to do so.

julianocosta89 · 2024-04-04T12:15:49Z

I already do my best to do so.

Appreciate it!

@Kimbohlovette would you be able to address #1477 (comment)?
I believe that if you are able to work out with it we will be able to have this one merged.

Kimbohlovette · 2024-04-04T14:29:49Z

@julianocosta89 yes I will continue to work with @pellared and definitely get it done

julianocosta89 · 2024-04-04T14:36:04Z

Feel free to re-open the issue whenever you have it in a working state

Kimbohlovette · 2024-04-04T14:57:47Z

Alright @julianocosta89

Kimbohlovette and others added 2 commits March 23, 2024 11:24

Added otellogrus to the accounting service

230b09a

Merge branch 'open-telemetry:main' into Add-otel-log-support-to-accou…

ec2ddb4

…ntingservice-1/7

Kimbohlovette requested a review from a team as a code owner March 23, 2024 13:01

propagated the context from sarama consumer group session into the lo…

f50deea

…g log.WithContext()

Kimbohlovette mentioned this pull request Mar 23, 2024

Add OpenTelemetry Log support to all services #1472

Open

8 tasks

Kimbohlovette changed the title ~~Add otel log support to accountingservice (1/7)~~ [accountingservice] adds otel logging support Mar 24, 2024

julianocosta89 reviewed Mar 25, 2024

View reviewed changes

puckpuck approved these changes Mar 25, 2024

View reviewed changes

pellared suggested changes Mar 25, 2024

View reviewed changes

austinlparker requested changes Mar 25, 2024

View reviewed changes

This was referenced Mar 25, 2024

[checkoutservice] adds otel logging support #1478

Closed

[productcatalogservice] adds otel log support #1479

Closed

[paymentservice] Adds otel logging support #1481

Closed

Replaced logrus with slog

6a5545a

Kimbohlovette marked this pull request as draft March 28, 2024 07:53

Merge branch 'main' into Add-otel-log-support-to-accountingservice-1/7

7c016d3

pellared reviewed Mar 28, 2024

View reviewed changes

Applied request changes/ initialized slog in main and removed initLog…

d138dc8

…ger()

Kimbohlovette requested a review from pellared March 28, 2024 17:42

pellared reviewed Apr 2, 2024

View reviewed changes

julianocosta89 closed this Apr 4, 2024

	logger.LogAttrs(ctx, slog.LevelError, "failed to initialize trace provider", slog.String("error", err.Error()))
	logger.LogAttrs(ctx, slog.LevelError, "Failed to initialize trace provider", slog.String("error", err.Error()))

	logger.Log(ctx, slog.LevelInfo, "message", "Accounting service exited")
	logger.LogAttrs(ctx, slog.LevelInfo, "Accounting service exited")

	logger.LogAttrs(ctx, slog.LevelInfo, "", slog.String("message", "Shotdown trace provider"))
	logger.LogAttrs(ctx, slog.LevelInfo, "Shutdown trace provider")

[accountingservice] adds otel logging support #1477

[accountingservice] adds otel logging support #1477

Conversation

Kimbohlovette commented Mar 23, 2024 • edited Loading

Kimbohlovette commented Mar 23, 2024

julianocosta89 left a comment

Choose a reason for hiding this comment

Kimbohlovette commented Mar 25, 2024

pellared left a comment

Choose a reason for hiding this comment

Kimbohlovette commented Mar 25, 2024

austinlparker left a comment

Choose a reason for hiding this comment

pellared commented Mar 25, 2024

Kimbohlovette commented Mar 25, 2024

pellared commented Mar 28, 2024

Kimbohlovette commented Mar 28, 2024

Kimbohlovette commented Mar 28, 2024 • edited Loading

pellared commented Mar 28, 2024

Kimbohlovette commented Mar 28, 2024 • edited Loading

pellared left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimbohlovette commented Mar 28, 2024

Kimbohlovette commented Mar 28, 2024 • edited Loading

pellared Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianocosta89 commented Apr 4, 2024

pellared commented Apr 4, 2024 • edited Loading

julianocosta89 commented Apr 4, 2024

pellared commented Apr 4, 2024

julianocosta89 commented Apr 4, 2024

Kimbohlovette commented Apr 4, 2024

julianocosta89 commented Apr 4, 2024

Kimbohlovette commented Apr 4, 2024

Kimbohlovette commented Mar 23, 2024 •

edited

Loading

Kimbohlovette commented Mar 28, 2024 •

edited

Loading

Kimbohlovette commented Mar 28, 2024 •

edited

Loading

Kimbohlovette commented Mar 28, 2024 •

edited

Loading

pellared Apr 2, 2024 •

edited

Loading

pellared Apr 2, 2024 •

edited

Loading

pellared commented Apr 4, 2024 •

edited

Loading