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

feat: slog: automatically reference transaction from context #881

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

adomaskizogian
Copy link
Contributor

@adomaskizogian adomaskizogian commented Mar 26, 2024

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

app, _ := newrelic.NewApplication(
  newrelic.ConfigAppName("foo"),
  newrelic.ConfigLicense("bar"),
)

instrumentedHandler := nrslog.WithTransactionFromContext(nrslog.TextHandler(app, os.Stderr, &slog.HandlerOptions{}))
slog.SetDefault(slog.New(instrumentedHandler))

on service.go

func DoSomeWork(ctx context.Context, app *newrelic.Application){
  txn := app.StartTransaction("baz")
  defer txn.End()

  ctx = newrelic.NewContext(ctx, txn)

  slog.InfoContext(ctx, "qux")  
}

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

@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

@adomaskizogian adomaskizogian changed the title feat: introduce TransactionFromContextHandler feat: automatically reference transaction from context on slog Mar 26, 2024
@adomaskizogian adomaskizogian changed the title feat: automatically reference transaction from context on slog feat: slog: automatically reference transaction from context Mar 26, 2024
@iamemilio iamemilio self-requested a review March 27, 2024 19:47
@iamemilio
Copy link
Contributor

@adomaskizogian thanks for the patch, is this ready for me to take a look? If not, @ me when it is.

@adomaskizogian
Copy link
Contributor Author

adomaskizogian commented Mar 27, 2024

@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

@iamemilio
Copy link
Contributor

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.

@iamemilio
Copy link
Contributor

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.

Copy link
Contributor

@iamemilio iamemilio left a 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

@nr-swilloughby nr-swilloughby merged commit 4134348 into newrelic:develop Apr 4, 2024
52 checks passed
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.

4 participants