newrelic / go-agent

New Relic Go Agent
Apache License 2.0
763 stars 297 forks source link

feat: slog: automatically reference transaction from context #881

Closed adomaskizogian closed 4 months ago

adomaskizogian commented 4 months ago

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 commented 4 months ago

CLA assistant check
All committers have signed the CLA.

iamemilio commented 4 months ago

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

adomaskizogian commented 4 months ago

@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 d8cab45ef887816bbf5b0f812496753bd4c4ba97

iamemilio commented 4 months ago

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 commented 4 months ago

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.