sdk: Support static track names in Java SDK#5219
Conversation
73e7551 to
a0bb642
Compare
670b295 to
d30c8bd
Compare
|
Ehm this PR requires some context, doesn't seem obvious at all to me. |
|
By default dynamic string track/counter names are ignored when uploading to westworld. The java sdk doesn't have any way to specify that the name is static and not dynamic, while there is a way for the c++ sdk to do so. This cl adds functions to register counter and track names with static names so that the lower layers can use this information. I'm making sure that @CompileTimeConstant is used to confirm that an input java string is infact static. eventually this calls the existing perfetto_protos_TrackDescriptor_set_cstr_static_name function. |
Ok I see, makes sense. I'll get to this tomorrow, I like the idea overall, but there are a few thing about how this is plumbed that require some more looking into |
|
I agree that this needs another look at the way it is plumbed. I'll see if I can reduce the code churn. Ill also split the PRs into two different PRs.
|
172312c to
41dc85e
Compare
|
Okay. cleaned up and split this PR into two |
|
My 2c: it would have nice if all the methods were inverted: that is, the static should be the default and the dynamic should need you to change the method name. |
Honestly, it's better now than later. I promise you we will really thank ourselves X years down the line when someone gets this wrong and it causes major issues :) |
|
Filed https://b/496954920. I'd like this followed up on but we don't need to block this CL. |
primiano
left a comment
There was a problem hiding this comment.
Hey sorry for the delay but i've been very backlogged.
I think the plumbing can be simplified if you make this a bool in the struct
cd24205 to
5e4c030
Compare
src/android_sdk/java/main/dev/perfetto/sdk/PerfettoTrackEventBuilder.java
Show resolved
Hide resolved
5e4c030 to
ad902ce
Compare
|
We had some discussion offline. What's confusing here is that there are two completely independent paths that we need to deal with
the macros are only involved into 1, and what you are doing there is great. My suggestion instead is that for 2, we should NOT plumb the boolean into PerfettoTeRegisteredTrackImpl as that is supposed to be used as an opaque state struct. Instead we should pass the boolean to PerfettoTeNamedTrackRegister and There deosn't seem to be any use outside of our repo yes, so we can freely add the boolean argument there. the rest of the CL looks good if we make that minor tweak (remember to update the api_integrationtest and shlib_example.c) |
032ba97 to
ab71e29
Compare
primiano
left a comment
There was a problem hiding this comment.
Thanks a lot for the patience here! LGTM
ab71e29 to
95d2863
Compare
Add new APIs for dynamic names Static track names are stored in field 10 of the TrackDescriptor protobuf, which allows the trace processor to use them as stable identifiers for tracks. Adds the following Java SDK APIs - usingNamedTrackWithDynamicName - usingProcessNamedTrackWithDynamicName - usingThreadNamedTrackWithDynamicName - usingCounterTrackWithDynamicName - usingProcessCounterTrackWithDynamicName - usingThreadCounterTrackWithDynamicName Adds the following Rust SDK APIs - set_named_track_with_dynamic_name - register_named_track_with_dynamic_name - register_counter_track_with_dynamic_name
95d2863 to
6373326
Compare
|
@dreveman PTAL. I need an owner's approval. |
By default dynamic string track/counter names are ignored when uploading to westworld.
The java sdk doesn't have any way to specify that the name is static and not dynamic, while there is a way for the c++ sdk to do so.
This cl adds functions to register counter and track names with static names so that the lower layers can use this information.
eventually this calls the existing perfetto_protos_TrackDescriptor_set_cstr_static_name function.
A follow up PR adds @CompileTimeConstant to confirm that an input java string is in fact static.