Add ILoggerProvider to capture error logs as APM logs#1135
Merged
gregkalapos merged 20 commits intoelastic:masterfrom Feb 16, 2021
gregkalapos:ErrorLogsAsApmErrors
Merged
Add ILoggerProvider to capture error logs as APM logs#1135gregkalapos merged 20 commits intoelastic:masterfrom gregkalapos:ErrorLogsAsApmErrors
gregkalapos merged 20 commits intoelastic:masterfrom
gregkalapos:ErrorLogsAsApmErrors
Conversation
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
Codecov Report
@@ Coverage Diff @@
## master #1135 +/- ##
==========================================
+ Coverage 81.10% 81.30% +0.19%
==========================================
Files 160 163 +3
Lines 6542 6681 +139
==========================================
+ Hits 5306 5432 +126
- Misses 1236 1249 +13
Continue to review full report at Codecov.
|
russcam
reviewed
Jan 20, 2021
Contributor
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
This looks good! I left some comments around naming
| <RootNamespace>Elastic.Apm.Extensions.Logging</RootNamespace> | ||
| <AssemblyName>Elastic.Apm.Extensions.Logging</AssemblyName> | ||
| <PackageId>Elastic.Apm.Extensions.Logging</PackageId> | ||
| <Description>TODO</Description> |
Contributor
There was a problem hiding this comment.
Might be worth adding a description now, so it's not forgotten 😃
src/Elastic.Apm.Extensions.Logging/Elastic.Apm.Extensions.Logging.csproj
Outdated
Show resolved
Hide resolved
| public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) | ||
| { | ||
| if (!Agent.IsConfigured) return; | ||
| if (logLevel < LogLevel.Error) return; |
Contributor
There was a problem hiding this comment.
Should this just use IsEnabled?
Contributor
Author
There was a problem hiding this comment.
Yes, good point. Fixed.
src/Elastic.Apm/Api/ITracer.cs
Outdated
| /// <param name="logOnError"> The logline itself </param> | ||
| /// <param name="parentId"> ParentId pointing to the parent transaction or span. </param> | ||
| /// <param name="exception"> Exception which was captured as part of the log. </param> | ||
| void CaptureLogAsError(ErrorLog logOnError, string parentId = null, Exception exception = null); |
Contributor
Author
There was a problem hiding this comment.
👍 This is a leftover from a previous renaming. Fixed.
src/Elastic.Apm/Api/ITracer.cs
Outdated
| /// <param name="logOnError"> The logline itself </param> | ||
| /// <param name="parentId"> ParentId pointing to the parent transaction or span. </param> | ||
| /// <param name="exception"> Exception which was captured as part of the log. </param> | ||
| void CaptureLogAsError(ErrorLog logOnError, string parentId = null, Exception exception = null); |
Contributor
There was a problem hiding this comment.
What are your thoughts here about introducing an overload to CaptureError for this? For example
void CaptureError(ErrorLog errorLog, string parentId = null, Exception exception = null);Capturing an error log, or message and culprit (and capturing an exception too) all map to the notion of an APM error, which could be surfaced in the API with an overloaded method.
Contributor
Author
There was a problem hiding this comment.
It's a reasonable approach, but I don't see a huge difference.
We already have CaptureError, CaptureException, and now we'd introduce CaptureLogAsError. With this I think the method name describes better what the intention is. I don't think using overloads would be very different.
|
|
||
| if (exception.StackTrace != null) | ||
| { | ||
| //TODO: |
Contributor
There was a problem hiding this comment.
still TODO or can be removed?
Contributor
Author
There was a problem hiding this comment.
Ah - yeah this was still ToDo - I implemented it.
test/Elastic.Apm.Extensions.Hosting.Tests/HostBuilderExtensionTests.cs
Outdated
Show resolved
Hide resolved
russcam
reviewed
Jan 21, 2021
src/Elastic.Apm/Api/Tracer.cs
Outdated
|
|
||
| public void CaptureError(string message, string culprit, StackFrame[] frames = null, string parentId = null) | ||
| { | ||
| var capturedCulprit = string.IsNullOrEmpty(culprit) ? "PublicAPI-CaptureException" : culprit; |
Contributor
There was a problem hiding this comment.
should this check to see if there's a current transaction or current span here, and correlate the error with it?
Contributor
Author
There was a problem hiding this comment.
Good catch! 👍 That's also how I documented it :) Fixed now.
src/Elastic.Apm/Api/Tracer.cs
Outdated
|
|
||
| public void CaptureException(Exception exception, string culprit = null, bool isHandled = false, string parentId = null) | ||
| { | ||
| var capturedCulprit = string.IsNullOrEmpty(culprit) ? "PublicAPI-CaptureException" : culprit; |
Contributor
There was a problem hiding this comment.
Same as above: should this check to see if there's a current transaction or current span here, and correlate the error with it?
src/Elastic.Apm/Api/Tracer.cs
Outdated
|
|
||
| public void CaptureLogAsError(ErrorLog errorLog, string parentId = null, Exception exception = null) | ||
| { | ||
| var error = new Error(errorLog, null, parentId, _logger) { Culprit = "Log" }; |
Contributor
There was a problem hiding this comment.
same as above: should this check to see if there's a current transaction or current span here, and correlate the error with it?
bmorelli25
reviewed
Jan 26, 2021
Member
bmorelli25
left a comment
bmorelli25
left a comment
There was a problem hiding this comment.
Docs LG! One recommendation – take it or leave it.
docs/public-api.asciidoc
Outdated
| ==== `void CaptureError(string message, string culprit, StackFrame[] frames = null, string parentId = null);` | ||
| Use this method to capture an APM error with a message and a culprit. | ||
|
|
||
| NOTE: If there is an active transaction, the captured error will be correlated to it, otherwise the error will show up, but it won't be correlated to a transaction on the APM UI. |
Member
There was a problem hiding this comment.
For the three identical NOTEs on this page, how do you feel about rewording to something like this?
Captured errors are automatically correlated with the active transaction. If no transaction is active, the error will still appear in the APM app but will not be correlated with a transaction.
Contributor
Author
There was a problem hiding this comment.
Thanks, yes, sounds better. I updated to this one.
russcam
reviewed
Feb 5, 2021
Contributor
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
I've left some comments, mainly around naming.
I think aligning method names on the concept of APM errors is useful i.e. an error in APM can be:
- A custom message, culprit and stack trace
- An exception
- An error level log event
docs/public-api.asciidoc
Outdated
|
|
||
| [float] | ||
| [[api-start-capture-log-as-error]] | ||
| ==== `void CaptureLogAsError(ErrorLog logOnError, string parentId = null, Exception exception = null);` |
Contributor
There was a problem hiding this comment.
Suggested change
| ==== `void CaptureLogAsError(ErrorLog logOnError, string parentId = null, Exception exception = null);` | |
| ==== `void CaptureLogAsError(ErrorLog errorLog, string parentId = null, Exception exception = null);` |
|
|
||
| namespace Elastic.Apm.Extensions.Logging | ||
| { | ||
| internal class ElasticApmErrorLogger : ILogger |
Contributor
There was a problem hiding this comment.
Thoughts on removing the ElasticApm prefix on the type name?
Contributor
Author
There was a problem hiding this comment.
My intention with this name was to express that this captures APM errors, which I think isn't obvious for an ILogger implementation. I added a comment and renamed to ApmErrorLogger (to include ApmError in the name).
| { | ||
| if (!Agent.IsConfigured) return; | ||
| if (!IsEnabled(logLevel)) return; | ||
| if (!_agent.ConfigurationReader.Enabled || !_agent.ConfigurationReader.Recording) return; |
Contributor
There was a problem hiding this comment.
This feels like an important thing to point out - error logs will only be captured by the APM agent if it's enabled and recording
Contributor
Author
There was a problem hiding this comment.
I added this to the Description of the package.
| if (!_agent.ConfigurationReader.Enabled || !_agent.ConfigurationReader.Recording) return; | ||
|
|
||
| var logLine = formatter(state, exception); | ||
| var logOnError = new ErrorLog(logLine); |
Contributor
There was a problem hiding this comment.
naming nitpick :)
Suggested change
| var logOnError = new ErrorLog(logLine); | |
| var errorLog = new ErrorLog(logLine); |
|
|
||
| namespace Elastic.Apm.Extensions.Logging | ||
| { | ||
| internal class ElasticApmErrorLoggingProvider : ILoggerProvider |
Contributor
There was a problem hiding this comment.
Thoughts on dropping the ElasticApm prefix?
| ); | ||
|
|
||
| /// <summary> | ||
| /// Captures a log line as an APM error. |
Contributor
There was a problem hiding this comment.
Should we adopt log event to describe a log?
Contributor
Author
There was a problem hiding this comment.
Yes. Seems MS docs also use log event. Changed it.
| HttpVersion = "2.0", | ||
| Socket = new Socket { Encrypted = true, RemoteAddress = "127.0.0.1" }, | ||
| Body = "123" | ||
| HttpVersion = "2.0", Socket = new Socket { Encrypted = true, RemoteAddress = "127.0.0.1" }, Body = "123" |
Contributor
Author
There was a problem hiding this comment.
Yeah, this was Rider, no real change here - I don't have an opinion on formatting - my only opinion is that it should be standardized and never be an issue :) I wonder why this happened on my machine and how we could standardize this. Maybe I'll check if .editorconfig could help here.
I changed this back to the original.
russcam
reviewed
Feb 15, 2021
docs/public-api.asciidoc
Outdated
|
|
||
| [float] | ||
| [[api-start-capture-error-log]] | ||
| ==== `void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null);` |
Contributor
There was a problem hiding this comment.
Suggested change
| ==== `void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null);` | |
| ==== `void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null);` |
docs/public-api.asciidoc
Outdated
| [[api-start-capture-error-log]] | ||
| ==== `void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null);` | ||
|
|
||
| Use this method to capture a log line as an APM error. |
Contributor
There was a problem hiding this comment.
Suggested change
| Use this method to capture a log line as an APM error. | |
| Use this method to capture a log event as an APM error. |
docs/public-api.asciidoc
Outdated
|
|
||
| [float] | ||
| [[api-transaction-capture-error-log]] | ||
| ==== `void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null);` |
Contributor
There was a problem hiding this comment.
Suggested change
| ==== `void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null);` | |
| ==== `void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null);` |
| /// <param name="parentId"> ParentId pointing to the parent transaction or span. </param> | ||
| /// <param name="exception"> Exception which was captured as part of the log. </param> | ||
| /// <param name="labels">Labels that will be added to the captured error</param> | ||
| void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null); |
Contributor
There was a problem hiding this comment.
Suggested change
| void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null); | |
| void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null); |
| /// <param name="serverInfo"></param> | ||
| /// <param name="exception"></param> | ||
| /// <param name="labels"></param> | ||
| internal static void CaptureLogAsError(ErrorLog logOnError, IPayloadSender payloadSender, IApmLogger logger, |
Contributor
There was a problem hiding this comment.
Suggested change
| internal static void CaptureLogAsError(ErrorLog logOnError, IPayloadSender payloadSender, IApmLogger logger, | |
| internal static void CaptureLogAsError(ErrorLog errorLog, IPayloadSender payloadSender, IApmLogger logger, |
src/Elastic.Apm/Model/NoopSpan.cs
Outdated
| Dictionary<string, Label> labels = null | ||
| ) { } | ||
|
|
||
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null | |
| public void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null |
| return false; | ||
| } | ||
|
|
||
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null | |
| public void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null |
src/Elastic.Apm/Model/Span.cs
Outdated
| public void SetLabel(string key, decimal value) | ||
| => Context.InternalLabels.Value.InnerDictionary[key] = value; | ||
|
|
||
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) | |
| public void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) |
src/Elastic.Apm/Model/Transaction.cs
Outdated
| return name; | ||
| } | ||
|
|
||
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureErrorLog(ErrorLog logOnError, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) | |
| public void CaptureErrorLog(ErrorLog errorLog, string parentId = null, Exception exception = null, Dictionary<string, Label> labels = null) |
russcam
reviewed
Feb 16, 2021
Comment on lines
+23
to
+24
| public IDisposable BeginScope<TState>(TState state) => | ||
| null; |
Contributor
There was a problem hiding this comment.
Even though this logger may not support scopes, should this return an instance of an IDisposable, so that usage via ILogger still works e.g.
private class NullScope: IDisposable
{
public NullScope() {}
public Dispose() {}
}
src/Elastic.Apm/Api/IError.cs
Outdated
| } | ||
|
|
||
| /// <summary> | ||
| /// Represents a log line which is captured as part of an APM error. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Represents a log line which is captured as part of an APM error. | |
| /// Represents a log event which is captured as part of an APM error. |
src/Elastic.Apm/Api/IError.cs
Outdated
| public string Message { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// A parametrized message. E.g. 'Could not connect to %s'. The property message is still required, and should be equal |
Contributor
There was a problem hiding this comment.
Suggested change
| /// A parametrized message. E.g. 'Could not connect to %s'. The property message is still required, and should be equal | |
| /// A parameterized message. E.g. 'Could not connect to %s'. The property message is still required, and should be equal |
src/Elastic.Apm/Api/IError.cs
Outdated
| /// A parametrized message. E.g. 'Could not connect to %s'. The property message is still required, and should be equal | ||
| /// to the param_message, but with placeholders replaced. In some situations the param_message is used to group errors | ||
| /// together. | ||
| /// The string is not interpreted, so feel free to use whichever placeholders makes sense in the client languange." |
Contributor
There was a problem hiding this comment.
Suggested change
| /// The string is not interpreted, so feel free to use whichever placeholders makes sense in the client languange." | |
| /// The string is not interpreted, so feel free to use whichever placeholders makes sense in the client language." |
src/Elastic.Apm/Api/ITracer.cs
Outdated
| /// <summary> | ||
| /// Captures a log event as an APM error. | ||
| /// </summary> | ||
| /// <param name="errorLog"> The log line itself </param> |
Contributor
There was a problem hiding this comment.
Suggested change
| /// <param name="errorLog"> The log line itself </param> | |
| /// <param name="errorLog"> The log event </param> |
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureLogAsErrorOnSpan() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureLogAsErrorOnSpan() | |
| public void CaptureErrorLogOnSpan() |
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureLogAsErrorOnTracer() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureLogAsErrorOnTracer() | |
| public void CaptureErrorLogOnTracer() |
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureLogOnTracerWithActiveSpan() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureLogOnTracerWithActiveSpan() | |
| public void CaptureErrorLogOnTracerWithActiveSpan() |
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureLogOnTracerWithActiveTransaction() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureLogOnTracerWithActiveTransaction() | |
| public void CaptureErrorLogOnTracerWithActiveTransaction() |
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureLogAsErrorOnTransaction() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void CaptureLogAsErrorOnTransaction() | |
| public void CaptureErrorLogOnTransaction() |
Adding new methods to capture errors.
Contributor
Author
|
I was watching CI for this to be green - next time please ping. |
Contributor
Author
|
It's late here, the last CI run almost finished, but due to the push it got cancelled. The 1 failing test seems to be unrelated. I'm hitting merge. Please next time ask before you push on other's branch. |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #894
Adding an
ILoggerProviderwhich captures logs on error level as APM Errors. By using theIHostBuilderextension method to enable the agent, we automatically turn on this feature and capture all error logs as APM errors.E.g.
(where
_loggeris a plainMicrosoft.Extensions.Loggiong.ILogger) will show up as a normal error:This PR also extends the public API
Capturing Exceptions and Errors directly on
ITracerIn case there is no current active span or transaction the error will show up on the UI, but won't be correlated to any transaction/span.
And also added log capturing as part of the public API:
Directly on tracer:
On a span:
On a transaction: