elastic / apm-agent-go

https://www.elastic.co/guide/en/apm/agent/go/current/index.html
Apache License 2.0
418 stars 193 forks source link

Slog Handler Elastic APM Integration #1597

Closed charliemenke closed 5 months ago

charliemenke commented 6 months ago

Noticed that there is no included "built-in-instrumentation" for the ever more popular slog structured logging package. Wrote up a mostly simple implementation of a slog Handler that:

Based my tests around the existing apmlogrus module so I hope I covered the most important cases.

LMK if I am missing anything critical, Thanks :)

cla-checker-service[bot] commented 6 months ago

💚 CLA has been signed

charliemenke commented 6 months ago

signed CLA,

signed again since I saw that my commit was using my short name

charliemenke commented 6 months ago

@dmathieu Thank you for the comprehensive and valuable review! It is much appreciated.

I understand the hesitancy to add new instrumentation since Elastic APM is moving towards Otel, so I will leave the decision up to you :). Though I will say since logging is still "unstable" it would be nice to have something supported directly in Elastic APM for such a core package like you said.

I responded to most of your review comments and pushed some changes to address them. I also left some comments where I am a bit unclear on the best way forward, namely when it comes to handling multiple "error" type attributes in one log message. Would love some feed back in those areas if we do decide to move forward with this.

Thanks again!

dmathieu commented 6 months ago

Note that there still seems to be a CLA issue.

charliemenke commented 6 months ago

just a bump from my end.

charliemenke commented 5 months ago

@dmathieu Thank you for your patience, I just pushed up changes to address the remaining comments.

FInally got the CLA to pass.

If all looks good I will update this branch with master as it looks like there has been changes since I last created it.

per my commit message:

Implemented ability to report multiple apm errors from one log. If a user adds
multiple "reportable" error attributes to the log msg (default is "error" & "err"),
instead of trying to join the errors into one or discarding one, the apmslog
handler will report both errors.

Added ability for a user to define what slog attribute keys they want to report
as errors. Because there is no standard way in slog to attach an error to a msg
log, I wanted to add the ability for the user to decide what is and what is not
going to be reported. By default, slog attribute keys that are "error" or "err"
are reported, but with the new `WithErrorRecordAttrs(keys)` function a user
can define which keys will be reported.

Cleaned up `ApmHandler` struct and methods. Since we want the user to use the
included `NewApmHandler` function and its functional option functions, I
decided to make all Struct fields private.

Additionally added a check on if the `ApmHandler`'s `tracer` field is nill before
trying to use it. It is still possible for a user to pass in a nil tracer using
the `WithTracer` functional option.
charliemenke commented 5 months ago

made the last changes and rebased into two relevant commits

dmathieu commented 5 months ago

run docs-build

charliemenke commented 5 months ago

Appreciate the reviews and all the suggestions! Have a good rest of the week :+1: