Skip to content

RUM-5985: Add isContainer attribute to session replay span#2244

Merged
ambushwork merged 1 commit into
developfrom
yl/profiling/add-containter-tag
Sep 6, 2024
Merged

RUM-5985: Add isContainer attribute to session replay span#2244
ambushwork merged 1 commit into
developfrom
yl/profiling/add-containter-tag

Conversation

@ambushwork

Copy link
Copy Markdown
Member

What does this PR do?

Add a "isContainer" attribute into session replay span request to differentiate the components and containers view.

Motivation

In the benchmark dashboard, we want to have a top list graph to see which components take the leads of the time consumption in SR. here is the current graph:
image

As we can see the top list is occupied by the containers, which doesn't what we expect. Because containers' spans contain the children view, so we want to have only real components here.

Demo

After adding this attributes, we can differentiate them by adding filter @attribute.container:false

Add we got pure components span in the list
image

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@ambushwork
ambushwork requested review from a team as code owners September 5, 2024 15:16
@ambushwork
ambushwork force-pushed the yl/profiling/add-containter-tag branch 3 times, most recently from 780d9b8 to ee1ed46 Compare September 5, 2024 15:45
@codecov-commenter

codecov-commenter commented Sep 5, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.99%. Comparing base (891909b) to head (73d8245).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
...id/sessionreplay/internal/recorder/BenchmarkExt.kt 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2244      +/-   ##
===========================================
- Coverage    70.00%   69.99%   -0.02%     
===========================================
  Files          727      728       +1     
  Lines        27041    27054      +13     
  Branches      4560     4559       -1     
===========================================
+ Hits         18930    18935       +5     
- Misses        6831     6843      +12     
+ Partials      1280     1276       -4     
Files with missing lines Coverage Δ
...essionreplay/internal/recorder/SnapshotProducer.kt 92.11% <100.00%> (ø)
...nternal/recorder/listener/WindowsOnDrawListener.kt 97.22% <100.00%> (ø)
...id/sessionreplay/internal/recorder/BenchmarkExt.kt 50.00% <50.00%> (ø)

... and 29 files with indirect coverage changes

@ambushwork
ambushwork force-pushed the yl/profiling/add-containter-tag branch from ee1ed46 to 8380c4f Compare September 6, 2024 07:34
/**
* A wrap function of [withinBenchmarkSpan] dedicated to session replay span recording.
*/
internal inline fun <T : Any?> withinBenchmarkSpan(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note

As it's a version dedicated for SR, and to easily disambiguate between this helper and the base method, maybe we could name it withinSRBenchmarkSpan instead, WDYT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea, updated

@ambushwork
ambushwork force-pushed the yl/profiling/add-containter-tag branch from 8380c4f to 73d8245 Compare September 6, 2024 12:25
@ambushwork
ambushwork requested review from 0xnm and xgouchet September 6, 2024 13:33
@ambushwork
ambushwork merged commit bf60244 into develop Sep 6, 2024
@ambushwork
ambushwork deleted the yl/profiling/add-containter-tag branch September 6, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants