-
Notifications
You must be signed in to change notification settings - Fork 12
feat: otlp logging added to config #599
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! The reason clue isn't using the log provider today is that it did not exist at the time clue was written so this PR is very welcomed :)
I left one question below: just wanting to understand why the recursive guard is needed. Thank you!
log/log.go
Outdated
|
|
||
| // Send to OpenTelemetry if available and not already logging to OTEL | ||
| if l.otellog != nil && !l.otelLogging { | ||
| // Set recursion guard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is the guard needed? Seems like we are protecting against otel calling back clue below, what else could cause recursion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the linter is picking up an issue that might be good to resolve before we merge, we probably also should have a few tests for the new Otel logger backend, thank you!
|
Hey, I went and added some additional fixes and adjustments. I don't think it is in a working state, but I wasn't able to test it. I am currently in the process of changing my logs over to |
This gives you the ability to send logs directly to the OpenTelemetry collector. Having traces and metrics go to the collector but not logs doesn't make sense to me. I could direct my stdout to a file but then I have to do additional parsing. I think it is a good idea to have this more natively part of clue.
As an aside though, do you think exporters should be more generic? Like making them a log option instead of an argument? Then you don't have to do a major version release every time you add a critical export.
Instead of
you could do
From
to