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

✨ Implement addTiming #668

Merged
merged 29 commits into from
Jan 11, 2021
Merged

✨ Implement addTiming #668

merged 29 commits into from
Jan 11, 2021

Conversation

webNeat
Copy link
Contributor

@webNeat webNeat commented Dec 28, 2020

Motivation

Add DD_RUM.addTiming() API.

Changes

Added the method addTiming to the global RUM object

function addTiming(name: string, inInitialView = false): void
  • name is the name of the custom timing. If the view already has that timing, it will be updated with the given time.
  • inInitialView indicates if we want to add the timing in the initial view. Otherwise the timing is added to the current view.

This new method is hidden behind the feature flag custom-timings.

Testing

  • Added automated tests
  • Tested with the local sandbox
  • Tested the facet list timing on the RUM Explorer. Got around 1.5 sec.

I have gone over the contributing documentation.

@webNeat webNeat requested a review from a team as a code owner December 28, 2020 18:08
@codecov-io
Copy link

codecov-io commented Jan 5, 2021

Codecov Report

Merging #668 (2ee6e03) into master (e2e99e4) will decrease coverage by 0.04%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   77.86%   77.81%   -0.05%     
==========================================
  Files          60       60              
  Lines        3266     3282      +16     
  Branches      710      714       +4     
==========================================
+ Hits         2543     2554      +11     
- Misses        723      728       +5     
Impacted Files Coverage Δ
packages/rum-core/src/rawRumEvent.types.ts 100.00% <ø> (ø)
packages/rum-core/test/fixtures.ts 100.00% <ø> (ø)
packages/rum-core/src/boot/rum.ts 50.00% <50.00%> (ø)
packages/core/src/tools/utils.ts 87.35% <100.00%> (+0.37%) ⬆️
packages/rum-core/src/boot/rumPublicApi.ts 93.90% <100.00%> (+0.15%) ⬆️
.../src/domain/rumEventsCollection/view/trackViews.ts 96.74% <100.00%> (+0.19%) ⬆️
.../domain/rumEventsCollection/view/viewCollection.ts 100.00% <100.00%> (ø)
packages/rum-core/src/transport/batch.ts 68.42% <0.00%> (-10.53%) ⬇️
...ckages/core/src/domain/automaticErrorCollection.ts 98.41% <0.00%> (-1.59%) ⬇️

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 e2e99e4...2ee6e03. Read the comment docs.

packages/rum/src/boot/rum.entry.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/boot/rum.entry.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/boot/rum.entry.ts Outdated Show resolved Hide resolved
sandbox/index.html Outdated Show resolved Hide resolved
addTiming: () => undefined,
configuration: ({
isEnabled: () => false,
} as any) as Configuration,
Copy link
Contributor

Choose a reason for hiding this comment

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

cf previous comment, any could be removed here too

const view = {
location,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
location,
location: location as Location,

it would remove the need to cast view as View in the next statement

@@ -0,0 +1,658 @@
/* tslint:disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

cf c822e93, it is not the right file

@@ -66,6 +66,7 @@ export function createRawRumEvent(type: RumEventType, overrides?: Context): RawR
date: 0,
view: {
action: { count: 0 },
customTimings: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

is it needed?

@webNeat webNeat merged commit 1687a37 into master Jan 11, 2021
@webNeat webNeat deleted the amine/add-timing branch January 11, 2021 14:52
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