Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

Log & Trace integration #104

Closed
wu-sheng opened this issue May 9, 2021 · 9 comments
Closed

Log & Trace integration #104

wu-sheng opened this issue May 9, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@wu-sheng
Copy link
Member

wu-sheng commented May 9, 2021

SkyWalking is enhancing its log integration capability, so, it will accept logs collected by popular tools, such as fluentd, filebeat.
It is better go2sky could inject the trace context information as a part of log text, which could make SkyWalking backend links logs and traces together.
You could take Java agent's implementation for a reference, http://skywalking.apache.org/docs/main/v8.5.0/en/setup/service-agent/java-agent/application-toolkit-log4j-1.x/#print-skywalking-context-in-your-logs

@wu-sheng wu-sheng added the enhancement New feature or request label May 9, 2021
@wu-sheng wu-sheng added this to the 1.1.0 milestone May 9, 2021
@mrproliu
Copy link
Contributor

I have done a little research, logrus and zap are more user use and popular framework on logging, also they use the MIT License, so we could integrate these frameworks.
Unfortunately, the Golang logging framework is not such as java has MDC or ThreadLocal mechanism, so we need to pass context data at all the log method invoke.

Logrus has the log format mechanism and transmits the context data when format. such as log.SetFormatter(&log.JSONFormatter{}), so we could wrap the format and adding the trace fields when the user invokes the log method with context data. When user integration, the user need to do two things:

  1. Setting customizes format, eg. log.SetFormatter(&logrusplugin.WrapFormat{base: &log.JSONFormatter}).
  2. Add context before log, eg. log.WithContext(ctx).Info("test").

Zap does not have the contextual metadata mechanism, also I find an issue is still open to tracking this. after reading this, I think we could have two ways to let user integration.

  1. Such as elastic apm, just create a built-in method for the user: https://www.elastic.co/guide/en/apm/agent/go/master/builtin-modules.html#builtin-modules-apmzap.
  2. Wrap the logging, eg. logging = zapplugin.WrapWithContext(logging). after the wrap, such as logrus, the user needs to add context before log or as an argument introduction when logging, eg. logging.Ctx(ctx).Info("test") or logging.Info(ctx, "test"). When the logging method has been invoking, we could add trace fields before logging.

Through these framework integrations, we could support add trace Id to the logging data. If we also need gRPC reporter, logrus we only need to create a setting method to connect logrus with our Tracer object, zap is the same thing(in this way, I suggest use wrap way to integration, so we could get more information and more control before logging).

@wu-sheng
Copy link
Member Author

About Zap, I think you could have both. Wrapper is easier to use for new codes, but (1) could change fewer existing codes, at least only change in one place.

@mrproliu
Copy link
Contributor

About Zap, I think you could have both. Wrapper is easier to use for new codes, but (1) could change fewer existing codes, at least only change in one place.

Of course. We could have both.

@wu-sheng
Copy link
Member Author

@mrproliu Sorry, I gave you a wrong reference. We could read 8.6.0-dev doc about context injection.

http://skywalking.apache.org/docs/main/latest/en/setup/service-agent/java-agent/application-toolkit-log4j-1.x/#print-skywalking-context-in-your-logs

Print SkyWalking context in your logs

@mrproliu
Copy link
Contributor

mrproliu commented May 17, 2021

@mrproliu Sorry, I gave you a wrong reference. We could read 8.6.0-dev doc about context injection.

http://skywalking.apache.org/docs/main/latest/en/setup/service-agent/java-agent/application-toolkit-log4j-1.x/#print-skywalking-context-in-your-logs

Print SkyWalking context in your logs

Ok, I think I need to expose some trace context method at go2sky project, such as SpanId(), ServiceInstanceName(). After this PR merged, I will continue working on the log plugins.

@wu-sheng
Copy link
Member Author

Which PR do you mean?

@mrproliu
Copy link
Contributor

Which PR do you mean?

Expose trace context data, current only exposing the trace id field, It needs to submit on the go2sky project. This PR I will submit recently.

@wu-sheng
Copy link
Member Author

Which PR do you mean?

Expose trace context data, current only exposing the trace id field, It needs to submit on the go2sky project. This PR I will submit recently.

Yes, let's go to that first.

@mrproliu
Copy link
Contributor

After the zap and logrus plugin merged. this issue could be close.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants