-
Notifications
You must be signed in to change notification settings - Fork 295
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
feat: slog: automatically reference transaction from context #881
Conversation
@adomaskizogian thanks for the patch, is this ready for me to take a look? If not, @ me when it is. |
it would be really helpful if you could share the details how to run tests of the nrslog package on local env. I would like to cover it better and make sure it's stable before it's ready for peer review. Also, I believe ci/cd workflow does not run the nrslog tests. Added it on d8cab45 |
I do most of my development in vscode, so I use the built in test tools to run targeted unit tests. Its much easier than learning the test CLI. Good spot on the CI missing the test though. |
You should not need anything special to run the tests though, and from what I can tell the test you created is structured correctly. I authorized the CI to run for you. |
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.
lgtm, thank you for this excellent contribution
I would like to introduce a handler that takes new relic transaction data from context automatically.
It is quite a common pattern to inject the base slog logger instance to some piece of code only so to pass it to
nrslog.WithContext
to receive a handler that captures logs in context. I find this api to be a bit leaky and introducing noise. Especially that slog already has methods which make context available for handlers.With introduction of this handler one now can set a slog instance relying on a NRHandler as default one and call it from anywhere, ensuring that log data with be passed to go-agent, as well as if the context contains transaction it will be linked. This is quite the case for http services which rely on
newrelic.WrapHandle
. This also decouples non infra/non instrumentation code from go-agent and allows to write code using stdlib.an example:
on main.go
on service.go
All feedback is welcome. The implementation itself was hacked out pretty quick. I am open for suggestions for taking a different approach such as re-implementing the handler. At the moment this new feature is implemented using a decorator which disables setting the transactions externally and only allows passing transaction via context.
I hope this will serve as a good starting point for a discussion. Ideally the resulting implementation will not require to change the example above and the usage remains the same.
btw, not sure if the tests will pass as I found no way to run tests on local env.
also, slack link is broken on CONTRIBUTING.md