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

Slog Handler Elastic APM Integration #1597

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

charliemenke
Copy link
Contributor

@charliemenke charliemenke commented Mar 25, 2024

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:

  • attaches any available trace/correlation ids to the log when using context aware log methods
  • reports specific log level logs as errors to Elastic APM

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 :)

  • Charlie

@charliemenke charliemenke requested a review from a team as a code owner March 25, 2024 01:24
Copy link

cla-checker-service bot commented Mar 25, 2024

💚 CLA has been signed

@charliemenke
Copy link
Contributor Author

charliemenke commented Mar 25, 2024

signed CLA,

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

@charliemenke charliemenke force-pushed the cmenke.slog-intrumentation branch 2 times, most recently from bc83d52 to 660b1b5 Compare March 25, 2024 15:18
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first feeling would be that we shouldn't add new instrumentations, since we're committing to OpenTelemetry.
But slog being a Go core logging library, I think having it makes sense.

Left a lot of comments, but the PR looks very nice overall. Thank you! 🌷

docs/supported-tech.asciidoc Outdated Show resolved Hide resolved
module/apmslog/example_test.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler.go Outdated Show resolved Hide resolved
module/apmslog/handler_test.go Outdated Show resolved Hide resolved
@charliemenke
Copy link
Contributor Author

charliemenke commented Mar 26, 2024

@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
Copy link
Member

dmathieu commented Apr 2, 2024

Note that there still seems to be a CLA issue.

@charliemenke
Copy link
Contributor Author

just a bump from my end.

  • I will look into the CLA issue, not sure why its not saying i signed...
  • ive been very busy with some other stuff this last week. Though I am still committed to working on this PR, hopefully will have some time soon to finish it up. Thanks for your patience :)

@charliemenke
Copy link
Contributor Author

charliemenke commented Apr 13, 2024

@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 charliemenke force-pushed the cmenke.slog-intrumentation branch 5 times, most recently from f394f1b to 043bfb8 Compare April 13, 2024 21:20
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into the sqlite errors. Overall, this looks great.

module/apmslog/handler_test.go Outdated Show resolved Hide resolved
module/apmslog/handler_test.go Outdated Show resolved Hide resolved
module/apmslog/handler_test.go Outdated Show resolved Hide resolved
cmenke added 2 commits April 22, 2024 15:24
Implemented a slog handler that attached trace/correlation logs (if
available) to the log message. Also will report specific log level logs
as errors through an apm tracer.

Add documentation links and clean up comments
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.

New tests and documentation added.
@charliemenke charliemenke force-pushed the cmenke.slog-intrumentation branch from 9eec9bd to 09e015c Compare April 22, 2024 21:25
@charliemenke
Copy link
Contributor Author

made the last changes and rebased into two relevant commits

@dmathieu
Copy link
Member

run docs-build

@dmathieu dmathieu merged commit 7d9fd78 into elastic:main Apr 24, 2024
14 checks passed
@charliemenke charliemenke deleted the cmenke.slog-intrumentation branch April 24, 2024 15:21
@charliemenke
Copy link
Contributor Author

Appreciate the reviews and all the suggestions! Have a good rest of the week 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants