Skip to content
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

Improve logs: Extra payload and open telemetry integration #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ilbambino
Copy link

Probably you don't want to land this as such. But we improved your package with a few things, that might be breaking changes.

  • Instead of using the contextKey trace to get the traceID integrate with OpenTelemetry and get it from the active Span (if any).
  • The formatting it was implemented didn't export the Data fields added to logrus. At least for us, that we use them a lot was a problem as lots of extra data was lost. To do it, instead of using the struct, it is using an intermediate map to be able to Marshal any possible field.

On the way I downgraded OpenTelemetry to 1.25 because the vendor we use still hasn't yet update to the latest and does not yet implement all the new interfaces.

Up to discuss if we can agree on common grounds to make this available for everyone

@ilbambino ilbambino changed the title Improve logs: Extra payload and open telemetry integration (#1) Improve logs: Extra payload and open telemetry integration Jun 20, 2024
Copy link
Owner

@tekkamanendless tekkamanendless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're going, but structurally, this isn't going to work right.

The general intent is that you would set up a single logrus logger in your application and then use a context.Context to drive exactly how it performs.

For example, each request will get its own context.Context via http.Request, but they'll all share the same global formatter. The context.Context carries the "context": logrus.WithContext(ctx).Infof("Some message") will have the appropriate information not because of the formatter, but because of the context.

Now, with that being said, you could restructure logEntry to be map[string]interface{} and add a couple more ContextKey constants (such as span, which I think is the only thing that you added that wasn't already there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants