Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ [RUMF-1181] preliminary refactorings for telemetry events collection #1371

Merged
merged 6 commits into from
Mar 3, 2022

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Mar 2, 2022

Motivation

Various refactoring mentioned in #1351 to ease the future collection of telemetry events

Changes

  • Move RUM batch creation earlier: in order to start collecting telemetry events as soon as possible
  • Mutualize monitoring batch and logger batch: easily reuse a simple batch with replica structure
  • Move RUM and Logs internal monitoring at same level: increase consistency between RUM and logs
  • Move internal monitoring transport to RUM and logs: telemetry transport strategy will vary depending on the package, so let the caller handle it

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@bcaudan bcaudan requested a review from a team as a code owner March 2, 2022 14:22
packages/rum-core/src/boot/startRum.ts Show resolved Hide resolved
@@ -95,6 +101,22 @@ export function startRum(
}
}

function startRumInternalMonitoring(configuration: RumConfiguration) {
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏This piece of code is now duplicated in both Logs and RUM. The goal is to be able to modify the RUM version to integrate it with LifeCycle in the follow-up PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, it the exact same thing for now but it will diverge to handle telemetry events

@codecov-commenter
Copy link

Codecov Report

Merging #1371 (f36f457) into main (6fb165b) will decrease coverage by 0.99%.
The diff coverage is 53.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
- Coverage   90.95%   89.95%   -1.00%     
==========================================
  Files         103      102       -1     
  Lines        4188     4182       -6     
  Branches      934      934              
==========================================
- Hits         3809     3762      -47     
- Misses        379      420      +41     
Impacted Files Coverage Δ
packages/rum-core/src/boot/startRum.ts 22.50% <0.00%> (-19.44%) ⬇️
packages/rum-core/src/transport/startRumBatch.ts 0.00% <ø> (-76.48%) ⬇️
packages/logs/src/boot/startLogs.ts 87.09% <73.33%> (-3.29%) ⬇️
...ckages/core/src/transport/startBatchWithReplica.ts 81.81% <81.81%> (ø)
...rc/domain/internalMonitoring/internalMonitoring.ts 98.30% <100.00%> (-0.11%) ⬇️
packages/rum-core/src/boot/rumPublicApi.ts 94.73% <100.00%> (-0.06%) ⬇️
...ages/rum-core/src/transport/startRumEventBridge.ts 0.00% <0.00%> (-100.00%) ⬇️
packages/core/src/transport/httpRequest.ts 76.66% <0.00%> (-20.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fb165b...f36f457. Read the comment docs.

@bcaudan bcaudan merged commit bd9e33f into main Mar 3, 2022
@bcaudan bcaudan deleted the bcaudan/preliminary branch March 3, 2022 10:05
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.

3 participants