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.

Sign up for GitHub

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

reporter: use semantic convention to report frame type #167

Merged
merged 7 commits into from
Oct 22, 2024
Merged

reporter: use semantic convention to report frame type #167

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8bd65dd
reporter: use semantic convention to report frame type
florianl Sep 24, 2024
dfeddf9
fixup: add new devfiler version
florianl Oct 3, 2024
31dde29
Merge branch 'main' into flo-drop-type-index
florianl Oct 3, 2024
d49c23d
fixup: drop -error and jitt suffix
florianl Oct 14, 2024
6a493db
fixup: filter APMInt frames
florianl Oct 14, 2024
b026f8e
Merge branch 'main' into flo-drop-type-index
florianl Oct 15, 2024
97c9727
Update libpf/interpretertype.go
florianl Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,10 @@ devfiler spins up a local server that listens on `0.0.0.0:11000`.

To run it, simply download and unpack the archive from the following URL:

https://upload.elastic.co/d/a3033fd2b515144b1c566961495302dac4da3223e59832f7755e18ac7fd94a19
https://upload.elastic.co/d/87e7697991940ec37f0c6e39ac38d213f65e8dc1ef9dbedff6aab9cba0adfaba

Authentication token: `c74dfc4db2212015`

Authentication token: `786a077d31b02bda`

The archive contains a build for each of the following platforms:

Expand Down
4 changes: 2 additions & 2 deletions libpf/interpretertype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ var interpreterTypeToString = map[InterpreterType]string{
UnknownInterp: "unknown",
PHP: "php",
PHPJIT: "phpjit",
Copy link
Member

@christos68k christos68k Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently there is no phpjit semantic convention, just php, so we should also introduce it (unless we think there is no value in the distinction between the two). It seems useful to keep the distinction.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. A lot of language runtimes have multiple execution tiers like "interpreted", "jit" or in the case of HotSpot even multiple tiers of JITing (C1 and C2). Perhaps instead of duplicating each frame kind, we should consider a separate attribute for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@christos68k Does it make sense to create a separate issue for this, so we can merge this PR. Without it, the devfiler won't work with the latest agent.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this and decided to wait until @florianl comes back as there's a decision to be made also for error frames and I lack context.

Copy link
Member

@christos68k christos68k Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We also discussed this today and think that it makes sense to introduce a new attribute for per-frame interpreter-specific metadata which may include:

  1. Error frame designation and error code
  2. Inline status
  3. JIT optimization / compiler level

We can discuss the specifics in the SIG.

As far as this PR is concerned, I think we can keep the frame type attribute and its string encoding, but this PR needs to be amended to be compliant with semantic conventions by:

  1. Sending PHPJIT frame types as "php" rather than "phpjit"
  2. Stripping error information (e.g. send "jvm" instead of "jvm-error")
  3. If we want to send abort frames we'd also need to define them in semconv

@florianl What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied the suggested changes.

Python: "python",
Python: "cpython",
Native: "native",
Kernel: "kernel",
HotSpot: "jvm",
Ruby: "ruby",
Perl: "perl",
V8: "v8",
V8: "v8js",
Dotnet: "dotnet",
APMInt: "apm-integration",
}
Expand Down
2 changes: 1 addition & 1 deletion libpf/libpf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestTraceType(t *testing.T) {
ty: PythonFrame,
isErr: false,
interp: Python,
str: "python",
str: "cpython",
},
{
ty: NativeFrame.Error(),
Expand Down
8 changes: 5 additions & 3 deletions reporter/otlp_reporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,13 +546,15 @@ func (r *OTLPReporter) getProfile() (profile *profiles.Profile, startTS, endTS u

// Walk every frame of the trace.
for i := range traceInfo.frameTypes {
frameAttributes := addProfileAttributes(profile, []attrKeyValue{
Copy link
Member

@christos68k christos68k Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This allows "abort-marker" (abortFrameName) to be a value that the agent sends with profile.frame.type, but there's no semantic convention for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christos68k Does it make sense to create a separate issue for this, so we can merge this PR. Without it, the devfiler won't work with the latest agent.

{key: "profile.frame.type", value: traceInfo.frameTypes[i].String()},
}, attributeMap)

loc := &profiles.Location{
// Id - Optional element we do not use.
TypeIndex: getStringMapIndex(stringMap,
traceInfo.frameTypes[i].String()),
Address: uint64(traceInfo.linenos[i]),
// IsFolded - Optional element we do not use.
// Attributes - Optional element we do not use.
Attributes: frameAttributes,
}

switch frameKind := traceInfo.frameTypes[i]; frameKind {
Expand Down