-
Notifications
You must be signed in to change notification settings - Fork 366
Sample frames to include column information in addition to line numbers #1014
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
Sample frames to include column information in addition to line numbers #1014
Conversation
Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>
|
Can you please also update coredumps to include the column information? Maybe something like this: |
… into column-info-with-profile-frames
This is done. |
|
@manojVivek coredump tests are failing now (as expected), as the column information is now also provided, but not present in files like https://github.com/open-telemetry/opentelemetry-ebpf-profiler/blob/main/tools/coredump/testdata/amd64/node211.json. |
(yeah, sorry for the ping before resolving that fully) I believe it should be passing now, it works on the arm Mac. Also updated the files similarly for the amd64, but didn't get a chance to test it locally so waiting for the CI run. |
fabled
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One nit. And question for all if the line/column data type could or should be changed to 32 bit size?
|
|
||
| // SourceColumn represents a column number within a source file line. It is intended to be used | ||
| // for the source column numbers in interpreted code. | ||
| type SourceColumn uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be made uint32 and the above SourceLineno also to reduce memory use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major concerns from me, except that otel protobuf spec has declared this as int64 too here: https://github.com/open-telemetry/opentelemetry-proto/blob/0d02f212598f3ec1dda35274e87f59351f619058/opentelemetry/proto/profiles/v1development/profiles.proto#L437
| lineNumber int64 | ||
| columnNumber int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here also, line/column probably could be int32?
interpreter/nodev8/v8.go
Outdated
| if index > 0 { | ||
| lineStart = lineEnds[index-1] + 1 | ||
| } else { | ||
| lineStart = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else branch is unneeded. line start is initialized to zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, fixed this.
… into column-info-with-profile-frames
Resolves #823
Contains: