Add CapturedStackFrame.ClassName #848
Merged
Conversation
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
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Log outputExpand to view the last 100 lines of log output
|
Contributor
Author
|
We discussed this and we'll go with the 3. option:
|
|
We are planning on implementing elastic/kibana#49467 for 7.10, where we will keep the existing output if we're missing a classname, and use the classname if we have it. |
Codecov Report
@@ Coverage Diff @@
## master #848 +/- ##
==========================================
+ Coverage 76.24% 79.85% +3.61%
==========================================
Files 144 146 +2
Lines 6866 7085 +219
==========================================
+ Hits 5235 5658 +423
+ Misses 1631 1427 -204
Continue to review full report at Codecov.
|
russcam
suggested changes
Nov 10, 2020
Contributor
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
I've left some comments. Introducing a type that encapsulates fetching and transforming stack traces may be useful
| /// <summary> | ||
| /// Encapsulates information about the APM Server that receives data from the agent. | ||
| /// </summary> | ||
| internal interface IServerInfo |
Contributor
There was a problem hiding this comment.
Might be worth naming this IApmServerInfo to indicate that it's information specific to APM server, as opposed to any other server e.g. server on which agent is running
Contributor
Author
There was a problem hiding this comment.
Agreed - implemented.
| /// <see cref="System.Version.Minor" /> | ||
| /// <see cref="System.Version.Build" /> | ||
| /// </summary> | ||
| public Version Version { get; } |
Contributor
There was a problem hiding this comment.
Will this ever need to take into account prerelease versions? If so, we might want to use a semantic version representation that accommodates prerelease suffixes. for example
// A simple version implementation based on
// https://github.com/maxhauser/semver/blob/master/src/Semver/SemVersion.cs
// MIT License
// Copyright (c) 2013 Max Hauser
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
using System;
using System.Globalization;
using System.Text.RegularExpressions;
namespace Apm.Agent
{
/// <summary>
/// An Elastic product version
/// </summary>
public sealed class ElasticVersion : IEquatable<ElasticVersion>, IComparable<ElasticVersion>, IComparable
{
private static readonly Regex VersionRegex = new Regex(
@"^(?<major>\d+)(\.(?<minor>\d+))?(\.(?<patch>\d+))?(\-(?<pre>[0-9A-Za-z]+))?$",
RegexOptions.CultureInvariant | RegexOptions.ExplicitCapture);
public ElasticVersion(string version)
{
var match = VersionRegex.Match(version);
if (!match.Success)
throw new ArgumentException($"Invalid version '{version}'", nameof(version));
var major = int.Parse(match.Groups["major"].Value, CultureInfo.InvariantCulture);
var minorMatch = match.Groups["minor"];
var minor = minorMatch.Success
? int.Parse(minorMatch.Value, CultureInfo.InvariantCulture)
: 0;
var patchMatch = match.Groups["patch"];
var patch = patchMatch.Success
? int.Parse(patchMatch.Value, CultureInfo.InvariantCulture)
: 0;
var prerelease = match.Groups["pre"].Value;
Major = major;
Minor = minor;
Patch = patch;
Prerelease = prerelease;
}
public ElasticVersion(int major, int minor, int patch, string prerelease)
{
Major = major;
Minor = minor;
Patch = patch;
Prerelease = prerelease;
}
public static bool TryParse(string version, out ElasticVersion elasticVersion)
{
try
{
elasticVersion = new ElasticVersion(version);
return true;
}
catch (Exception)
{
elasticVersion = null;
return false;
}
}
public static bool Equals(ElasticVersion versionA, ElasticVersion versionB) =>
ReferenceEquals(versionA, null)
? ReferenceEquals(versionB, null)
: versionA.Equals(versionB);
public static int Compare(ElasticVersion versionA, ElasticVersion versionB) =>
ReferenceEquals(versionA, null)
? ReferenceEquals(versionB, null) ? 0 : -1
: versionA.CompareTo(versionB);
public ElasticVersion Change(int? major = null, int? minor = null, int? patch = null, string prerelease = null) =>
new ElasticVersion(
major ?? Major,
minor ?? Minor,
patch ?? Patch,
prerelease ?? Prerelease);
public int Major { get; }
public int Minor { get; }
public int Patch { get; }
public string Prerelease { get; }
public override string ToString()
{
var version = "" + Major + "." + Minor + "." + Patch;
if (!string.IsNullOrEmpty(Prerelease))
version += "-" + Prerelease;
return version;
}
public int CompareTo(object obj) => CompareTo((ElasticVersion)obj);
public int CompareTo(ElasticVersion other)
{
if (ReferenceEquals(other, null))
return 1;
var r = CompareByPrecedence(other);
return r;
}
public bool PrecedenceMatches(ElasticVersion other) => CompareByPrecedence(other) == 0;
public int CompareByPrecedence(ElasticVersion other)
{
if (ReferenceEquals(other, null))
return 1;
var r = Major.CompareTo(other.Major);
if (r != 0) return r;
r = Minor.CompareTo(other.Minor);
if (r != 0) return r;
r = Patch.CompareTo(other.Patch);
if (r != 0) return r;
r = CompareComponent(Prerelease, other.Prerelease, true);
return r;
}
private static int CompareComponent(string a, string b, bool lower = false)
{
var aEmpty = string.IsNullOrEmpty(a);
var bEmpty = string.IsNullOrEmpty(b);
if (aEmpty && bEmpty)
return 0;
if (aEmpty)
return lower ? 1 : -1;
if (bEmpty)
return lower ? -1 : 1;
var aComps = a.Split('.');
var bComps = b.Split('.');
var minLen = Math.Min(aComps.Length, bComps.Length);
for (int i = 0; i < minLen; i++)
{
var ac = aComps[i];
var bc = bComps[i];
int anum, bnum;
var isanum = int.TryParse(ac, out anum);
var isbnum = int.TryParse(bc, out bnum);
int r;
if (isanum && isbnum)
{
r = anum.CompareTo(bnum);
if (r != 0) return anum.CompareTo(bnum);
}
else
{
if (isanum)
return -1;
if (isbnum)
return 1;
r = string.CompareOrdinal(ac, bc);
if (r != 0)
return r;
}
}
return aComps.Length.CompareTo(bComps.Length);
}
public override bool Equals(object obj)
{
if (ReferenceEquals(obj, null))
return false;
if (ReferenceEquals(this, obj))
return true;
var other = (ElasticVersion)obj;
return Equals(other);
}
public bool Equals(ElasticVersion other)
{
if (other == null)
return false;
return Major == other.Major &&
Minor == other.Minor &&
Patch == other.Patch &&
string.Equals(Prerelease, other.Prerelease, StringComparison.Ordinal);
}
public override int GetHashCode()
{
unchecked
{
int result = Major.GetHashCode();
result = result * 31 + Minor.GetHashCode();
result = result * 31 + Patch.GetHashCode();
result = result * 31 + Prerelease.GetHashCode();
return result;
}
}
public static bool operator ==(ElasticVersion left, ElasticVersion right) => Equals(left, right);
public static bool operator !=(ElasticVersion left, ElasticVersion right) => !Equals(left, right);
public static bool operator >(ElasticVersion left, ElasticVersion right) => Compare(left, right) > 0;
public static bool operator >=(ElasticVersion left, ElasticVersion right) => left == right || left > right;
public static bool operator <(ElasticVersion left, ElasticVersion right) => Compare(left, right) < 0;
public static bool operator <=(ElasticVersion left, ElasticVersion right) => left == right || left < right;
}
}
Contributor
Author
There was a problem hiding this comment.
Good point - especially because the APM Server sends semantic versions, so we should parse it accordingly.
To the question:
Will this ever need to take into account prerelease versions?
I don't think it will, but that's just a guess, maybe it will - but for the parsing we need to take this into consideration.
So I took this class and added it to the PR.
| public bool ServerVersionQueried => _initialized; | ||
| public Version Version { get; private set; } | ||
|
|
||
| public async Task GetServerInfoAsync() |
Contributor
There was a problem hiding this comment.
Should the fetching of server info (behaviour) be separate from the data?
Contributor
Author
There was a problem hiding this comment.
I separated the fetching logic - one related problem I found is that if an API keys or Secret token is set, then it should be also used for the Server Info API, otherwise the APM server won't respond.
| }); | ||
| }; | ||
|
|
||
| if (serverInfo.ServerVersionQueried && serverInfo.Version != null && serverInfo.Version < V710) |
Contributor
There was a problem hiding this comment.
Thoughts on having an IStackTrace[Generator|Transformer] with two different implementations to perform the relevant changes to CapturedStackFrame?
Contributor
Author
There was a problem hiding this comment.
For the changes in this PR I would not do it - long term this if block will go away. We can completely remove this part once 7.9 is out of support.
But in general I agree there is a potential to better structure the stack trace capturing logic. Happy to make follow ups, but for solving the current problem from this PR I think this is fine.
Co-authored-by: Russ Cam <russ.cam@forloop.co.uk>
Contributor
|
jenkins run the tests please |
Contributor
|
tests look like they're timing out on this. Can we bump the timeout, @gregkalapos? |
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.
Solves #677
Language specific stack traces elastic/kibana#49467 was added in
7.10in Kibana.This PR utilizes that feature.
Prior to this PR
Pre
7.10, Kibana always showed stack traces in the following format:[Filename] in [MethodName] at line [LineNo].For .NET this was very problematic, because that would have meant we never show stack traces in callstacks, furthermore, sometimes only the assembly name would be the only filename we could get, so lots of frames would be something like
System.Private.CoreLib.dll in SomeMethod().As a workaround we (...well I) had the idea to use the
FileNameto store the real class name. This sounds very dirty (and it is), but I strongly think it's still better to show proper call stacks and include theClassNamealways.Solution
As stated in #677 APM Server introduced a new field to store class names and in
7.10Kibana uses language specific call stacks where for .NET it also shows the class name. Details: elastic/kibana#49467What we do now
If the agent connects to APM Server version
7.10or newer, we send both FileName and ClassName and Kibana will show a stacktrace with format:[ClassName] in [MethodName] in [FileName] at line [LineNo]. 🎉But we need to maintain backwards compatibility, so for pre 7.10 versions we still use the hack described above. Once
7.9is out of support we can remove this code.So this PR does basically 2 things: