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

Set stacktrace.abs_path #856

Closed
wants to merge 2 commits into from

Conversation

felixbarny
Copy link
Member

To make the integration with the Code app easier, they need the abs_path of the stack frame. In Java it's impossible to determine the exact path within the root of the git repository, like apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java but based on the package name and file name, we can get something like co/elastic/apm/agent/report/serialize/DslJsonSerializer.java.

The question is if it's ok to put a relative path in abs_path.

Checklist

@felixbarny felixbarny self-assigned this Sep 20, 2019
@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

Merging #856 into master will increase coverage by 0.6%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #856     +/-   ##
===========================================
+ Coverage     64.22%   64.83%   +0.6%     
  Complexity       84       84             
===========================================
  Files           230      230             
  Lines          9329     9449    +120     
  Branches       1214     1245     +31     
===========================================
+ Hits           5992     6126    +134     
+ Misses         2956     2945     -11     
+ Partials        381      378      -3
Impacted Files Coverage Δ Complexity Δ
.../apm/agent/report/serialize/DslJsonSerializer.java 89.71% <0%> (+3.62%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 519fb27...4d1eab9. Read the comment docs.

serializeStackFrameModule(stacktrace.getClassName());
int lastDotIndex = className.lastIndexOf('.');
if (lastDotIndex > 0) {
writeField("abs_path", className.substring(0, lastDotIndex).replace('.', '/') + "/" + stacktrace.getFileName());
Copy link
Contributor

Choose a reason for hiding this comment

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

This allocates two String objects (substring() and replace()) for each stack frame of each span. Can we use a char-copy to a StringBuilder that includes the char replacement as well?
If so, can it also be used for handling inner classes, lambdas and the like or that using getFileName() will always be safer?

@felixbarny
Copy link
Member Author

We decided in today's meeting that it's not a good idea to put a relative path in a field called abs_path. For now, the code search integration should rely on just the file name, even if it may cause ambiguities.

We discovered discrepancies between the agent implementations as some put a relative path in the filename and some only put the base path in there.

Going forward, we should investigate alignment with ECS for stack traces. There are discussions going on at elastic/ecs#154 and elastic/ecs#563.

@felixbarny felixbarny closed this Oct 2, 2019
@felixbarny felixbarny deleted the stacktrace.abs_path branch October 2, 2019 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants