reporter: use semantic convention to report frame type #167
Conversation
With open-telemetry/semantic-conventions#1188 a semantic convention for frame types was defined and open-telemetry/opentelemetry-proto#578 removed Location.type_index from the OTel profiling signal in favor of the semantic convention. Update the reporter to use this semantic convention. Signed-off-by: Florian Lehner <[email protected]>
florianl
force-pushed
the
flo-drop-type-index
branch
from
162de1f to
8bd65dd
Compare
|
I did run into issues when creating a new |
Signed-off-by: Florian Lehner <[email protected]>
|
sorry again for the delay 🙏 PR got updated and provides now a link in the README to the new devfiler version |
libpf/interpretertype.go
Outdated
| @@ -55,13 +55,13 @@ var interpreterTypeToString = map[InterpreterType]string{ | |||
| UnknownInterp: "unknown", | |||
| PHP: "php", | |||
| PHPJIT: "phpjit", | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Error frame designation and error code
- Inline status
- 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:
- Sending PHPJIT frame types as "php" rather than "phpjit"
- Stripping error information (e.g. send "jvm" instead of "jvm-error")
- If we want to send abort frames we'd also need to define them in semconv
@florianl What do you think?
There was a problem hiding this comment.
applied the suggested changes.
| @@ -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{ | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
Co-authored-by: Christos Kalkanis <[email protected]>
Signed-off-by: Florian Lehner <[email protected]> Co-authored-by: Christos Kalkanis <[email protected]>
With open-telemetry/semantic-conventions#1188 a semantic convention for frame types was defined and open-telemetry/opentelemetry-proto#578 removed Location.type_index from the OTel profiling signal in favor of the semantic convention.
Update the reporter to use this semantic convention.