Add tracing to Digital Credentials API get() flow [chromium/src : main]
Mohamed Amir Yosef (Gerrit)
Mohamed Amir Yosef added 4 comments![]( "Open in Gerrit")
I am very sorry for the delay!
I have added some comments!
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",
static_cast<int>(status), "request_type",
static_cast<int>(request_type), "protocol", protocol);This is invoked for both create and get requests.
IMHO, it's better to filter here already!
UseCounter::Count(resolver->GetExecutionContext(),
WebFeature::kIdentityDigitalCredentialsSuccess);Unrelated but since we started the issuance OT, could you please split this too for get() and create() in a separate CL?
Related details
- Anushka Kulkarni
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 7 comments![]( "Open in Gerrit")
"Traces for the Digital Credentials API"),shouldn't this be indented?
perfetto::Category("content.digitalcredentials").SetDescription(I would put this before content.fedcm because d comes before f alphabetically :)
TRACE_EVENT0(AFAIK this is the legacy version of the macro and current versions should TRACE_EVENT(...). https://source.chromium.org/chromium/chromium/src/+/main:v8/src/tracing/trace-event-no-perfetto.h;l=104?q=TRACE_EVENT0&ss=chromium
TRACE_EVENT1("content.digitalcredentials", "DigitalIdentityRequestImpl::Get",same here
TRACE_EVENT0("content.digitalcredentials",same here
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",here you do use the newer version already...?
TRACE_EVENT0("content.digitalcredentials",and here
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 10 comments![]( "Open in Gerrit")
shouldn't this be indented?
Done
perfetto::Category("content.digitalcredentials").SetDescription(I would put this before content.fedcm because d comes before f alphabetically :)
Done
AFAIK this is the legacy version of the macro and current versions should TRACE_EVENT(...). https://source.chromium.org/chromium/chromium/src/+/main:v8/src/tracing/trace-event-no-perfetto.h;l=104?q=TRACE_EVENT0&ss=chromium
Done
Anushka Kulkarninit: revert this please
Done
TRACE_EVENT1("content.digitalcredentials", "DigitalIdentityRequestImpl::Get",Anushka Kulkarnisame here
Done
TRACE_EVENT0("content.digitalcredentials",Anushka Kulkarnisame here
Done
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",
static_cast<int>(status), "request_type",
static_cast<int>(request_type), "protocol", protocol);This is invoked for both create and get requests.
IMHO, it's better to filter here already!
Done
UseCounter::Count(resolver->GetExecutionContext(),
WebFeature::kIdentityDigitalCredentialsSuccess);Unrelated but since we started the issuance OT, could you please split this too for get() and create() in a separate CL?
The required changes have already been made
TRACE_EVENT0("content.digitalcredentials",Anushka Kulkarniand here
Done
Related details
- Christian Biesinger
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",so this is now scoped to the if, whereas the other trace events cover the rest of the function.
If this is intentionally scoped to the if, `TRACE_EVENT_INSTANT` seems clearer.
if not, I am not sure what the best way to handle that is; TRACE_EVENT_BEGIN/END would work but seems not ideal
Related details
- Anushka Kulkarni
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 1 comment![]( "Open in Gerrit")
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",so this is now scoped to the if, whereas the other trace events cover the rest of the function.
If this is intentionally scoped to the if, `TRACE_EVENT_INSTANT` seems clearer.
if not, I am not sure what the best way to handle that is; TRACE_EVENT_BEGIN/END would work but seems not ideal
Changed to TRACE_EVENT_INSTANT since the event is scoped to the if-block and doesn't measure the function's actual work; this clarifies it's marking a point in time, not a duration.
Related details
- Christian Biesinger
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 2 comments![]( "Open in Gerrit")
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",
static_cast<int>(status), "request_type",
static_cast<int>(request_type), "protocol", protocol);Anushka KulkarniThis is invoked for both create and get requests.
IMHO, it's better to filter here already!
Done
@Mohamed Amir Yosef,Removing the kGet guard so the trace is emitted for both Get and Create flows, since OnCompleteRequest is invoked by both APIs. This ensures the event is recorded whenever the corresponding DC API is called. Also, placing the trace inside the if block would only capture the scope of that conditional rather than reflecting the overall execution of OnCompleteRequest.The CL for create flow is in progress.
TRACE_EVENT("content.digitalcredentials", "OnCompleteRequest", "status",Anushka Kulkarniso this is now scoped to the if, whereas the other trace events cover the rest of the function.
If this is intentionally scoped to the if, `TRACE_EVENT_INSTANT` seems clearer.
if not, I am not sure what the best way to handle that is; TRACE_EVENT_BEGIN/END would work but seems not ideal
Changed to TRACE_EVENT_INSTANT since the event is scoped to the if-block and doesn't measure the function's actual work; this clarifies it's marking a point in time, not a duration.
@Christian Biesinger,Removing the kGet guard so the trace is emitted for both Get and Create flows, since OnCompleteRequest is invoked by both APIs. This ensures the event is recorded whenever the corresponding DC API is called.The CL for create flow is in progress.
Related details
- Christian Biesinger
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christian Biesinger
| Code-Review | +1 |
2 comments
lgtm but please let mamir take a look as well
This CL introduces Perfetto-based tracing in the Digital Credentials API browser-side implementation for the get() method. The tracing captures critical points in the request lifecycle to aid debugging and performanceplease wrap this line. you can use gerrit's Format feature.
Related details
- Anushka Kulkarni
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
Mohamed Amir Yosef added 1 comment![]( "Open in Gerrit")
TRACE_EVENT("content.digitalcredentials",
"AuthenticationCredentialsContainer::get");`AuthenticationCredentialsContainer::get()` is invoked for many other flows in addition to Digital Credentials (e.g. Passkeys)
Would it make more sense to move this to 1343?
And in fact in this case, it is right before invoking `DiscoverDigitalIdentityCredentialFromExternalSource()` which is already annotated.
So maybe this is not needed altogether?
WDYT?
Related details
- Anushka Kulkarni
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 2 comments![]( "Open in Gerrit")
This CL introduces Perfetto-based tracing in the Digital Credentials API browser-side implementation for the get() method. The tracing captures critical points in the request lifecycle to aid debugging and performanceplease wrap this line. you can use gerrit's Format feature.
Done
TRACE_EVENT("content.digitalcredentials",
"AuthenticationCredentialsContainer::get");`AuthenticationCredentialsContainer::get()` is invoked for many other flows in addition to Digital Credentials (e.g. Passkeys)
Would it make more sense to move this to 1343?
And in fact in this case, it is right before invoking `DiscoverDigitalIdentityCredentialFromExternalSource()` which is already annotated.
So maybe this is not needed altogether?
WDYT?
@Mohamed Amir Yosef, thanks for pointing that out! I agree with your recommendation — having the trace at the top-level get() would incorrectly tag non-DC flows under the Digital Credentials category, which could skew metrics and introduce noise. Made the required changes.
Related details
- Christian Biesinger
- Mohamed Amir Yosef
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |