Stream background traces to multiple RNDT sessions if present#55351
Closed
motiz88 wants to merge 4 commits into
Closed
Stream background traces to multiple RNDT sessions if present#55351motiz88 wants to merge 4 commits into
motiz88 wants to merge 4 commits into
Conversation
meta-cla
Bot
added
the
CLA Signed
motiz88
force-pushed
the
export-D90888852
branch
2 times, most recently
from
be2dfb8 to
3c335e1
Compare
…ets (react#55314) Summary: TSIA - minor refactor for correctness and clarity before making changes to some of these methods up the stack. Changelog: [Internal] Reviewed By: hoxyq Differential Revision: D91140491
Summary: Changelog: [Internal] TSIA - minor refactor for convenience and correctness. NOTE: When we get C++23, we can deduplicate the identical const/non-const method bodies using the nifty [deducing `this`](https://en.cppreference.com/w/cpp/language/member_functions.html#Explicit_object_member_functions) feature. Reviewed By: hoxyq Differential Revision: D91140490
) Summary: Changelog: [Internal] Moves state and logic related to stashing a background trace from `JReactHostInspectorTarget` (JNI wrapper boilerplate, Android-specific) to `HostTarget` (cross-platform C++). Managing the full lifecycle of a background trace inside `HostTarget` lets us remove a fair amount of API surface: * `HostTargetDelegate::unstable_getHostTracingProfileThatWillBeEmittedOnInitialization()` * `HostTarget::hasActiveSessionWithFuseboxClient()` * `TracingAgent::emitExternalHostTracingProfile()` We also refactor some of the surrounding code and add tests (previously missing) for the behaviour of stashed background traces when there is more than one session. Reviewed By: huntie, hoxyq Differential Revision: D91589884
…55351) Summary: Changelog: [Internal] ## Behaviour Fixes an edge case in the way background traces are emitted when multiple RNDT frontends (or other clients with the `ReactNativeApplication` domain enabled) are connected to a single Host. Previously, only one of the clients would receive the CDP events for the trace. With this diff, *all* of them will. NOTE: This leaves another, even smaller edge case / inconsistency: if there are *no* RNDT frontends at the time a background trace ends, we still only send it to the first frontend (if any) that connects. This is logically more defensible than picking one client out of multiple active connections ( = what's fixed in this diff). ## Implementation Tracing functionality is somewhat awkwardly split between `HostTarget` (background traces) and `TracingAgent` (CDP-initiated traces). This diff doesn't attempt to fully clean this up[1], but we do reduce duplication and boilerplate by creating a helper function used by both. This function encapsulates the gnarly/surprising details of dealing with `tracing::HostTracingProfile` objects in a memory-efficient way: 1. As per the existing implementation, we serialise them to JSON chunk-by-chunk, destroying the original object in the process. 2. Each chunk is sent to all relevant sessions *and destroyed* before we begin serialising the next one. [1] In future work, we should probably move the Host's tracing state and logic into its own `TracingTarget` class. The naturally close coupling of a Target with its corresponding Agent would be helpful here. Reviewed By: huntie Differential Revision: D90888852
motiz88
force-pushed
the
export-D90888852
branch
from
3c335e1 to
4382a95
Compare
Collaborator
|
This pull request was successfully merged by @motiz88 in a640790 When will my fix make it into a release? | How to file a pick request? |
|
This pull request has been merged in a640790. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Changelog: [Internal]
Behaviour
Fixes an edge case in the way background traces are emitted when multiple RNDT frontends (or other clients with the
ReactNativeApplicationdomain enabled) are connected to a single Host. Previously, only one of the clients would receive the CDP events for the trace. With this diff, all of them will.NOTE: This leaves another, even smaller edge case / inconsistency: if there are no RNDT frontends at the time a background trace ends, we still only send it to the first frontend (if any) that connects. This is logically more defensible than picking one client out of multiple active connections ( = what's fixed in this diff).
Implementation
Tracing functionality is somewhat awkwardly split between
HostTarget(background traces) andTracingAgent(CDP-initiated traces). This diff doesn't attempt to fully clean this up[1], but we do reduce duplication and boilerplate by creating a helper function used by both. This function encapsulates the gnarly/surprising details of dealing withtracing::HostTracingProfileobjects in a memory-efficient way:[1] In future work, we should probably move the Host's tracing state and logic into its own
TracingTargetclass. The naturally close coupling of a Target with its corresponding Agent would be helpful here.Differential Revision: D90888852