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-530][Logs] allow using logs API before init #545

Merged
merged 22 commits into from
Sep 30, 2020

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Sep 24, 2020

Motivation

We want to allow using log APIs before initialising the SDK.

Changes

  • Remove stubs and regroup the public API implementation
  • Introduce a 'buffered function' to buffer pre-ini function calls
  • Use a buffered function to send logs

Testing

Unit tests have been updated. Manual testing can be done by running some DD_LOGS.logger.log('foo') before DD_LOGS.init().


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner September 24, 2020 13:08
@BenoitZugmeyer BenoitZugmeyer changed the title Benoit/allow api call before init logs ✨ [RUMF-530][Logs] allow using logs API before init Sep 24, 2020
packages/core/src/cookie.ts Outdated Show resolved Hide resolved
packages/logs/src/logs.entry.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2020

Codecov Report

Merging #545 into master will increase coverage by 1.55%.
The diff coverage is 91.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
+ Coverage   88.11%   89.66%   +1.55%     
==========================================
  Files          36       38       +2     
  Lines        2288     2265      -23     
  Branches      469      473       +4     
==========================================
+ Hits         2016     2031      +15     
+ Misses        272      234      -38     
Impacted Files Coverage Δ
packages/core/src/configuration.ts 91.66% <ø> (-0.27%) ⬇️
packages/logs/src/logs.ts 74.19% <74.19%> (ø)
packages/core/src/boundedBuffer.ts 100.00% <100.00%> (ø)
packages/core/src/init.ts 81.81% <100.00%> (+1.81%) ⬆️
packages/logs/src/logger.ts 100.00% <100.00%> (+4.59%) ⬆️
packages/logs/src/logs.entry.ts 100.00% <100.00%> (+50.74%) ⬆️
packages/rum/src/rum.entry.ts 74.62% <100.00%> (ø)
packages/rum/src/performanceCollection.ts 68.57% <0.00%> (+1.42%) ⬆️
... and 1 more

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 81fffee...316510b. Read the comment docs.

packages/core/src/boundedBuffer.ts Outdated Show resolved Hide resolved
packages/logs/src/logger.ts Show resolved Hide resolved
packages/logs/src/logs.ts Outdated Show resolved Hide resolved
packages/logs/src/logger.ts Show resolved Hide resolved
packages/logs/test/logs.spec.ts Outdated Show resolved Hide resolved
packages/logs/src/logs.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

Nice job 👍

@BenoitZugmeyer BenoitZugmeyer merged commit 27f4afa into master Sep 30, 2020
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/allow-api-call-before-init-logs branch September 30, 2020 13:47
@@ -5,7 +5,6 @@ import { includes, ONE_KILO_BYTE, ONE_SECOND } from './utils'

export const DEFAULT_CONFIGURATION = {
allowedTracingOrigins: [] as Array<string | RegExp>,
isCollectingError: true,

Choose a reason for hiding this comment

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

Just letting you know this is a breaking change but wasn't reported as such in your changelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @tbillington, it should not be.
There was an issue in the logs documentation, isCollectingError was not an init parameter for logs, forwardErrorToLogs was the right one: https://github.com/DataDog/browser-sdk/pull/545/files#diff-2e4bca1144de53c8c504edb040ac34aa7ea63ba413a4453277d88307278d8972R43.
It is still collecting error by default: https://github.com/DataDog/browser-sdk/pull/545/files#diff-c62c675364c11388aeae30517afdac97f57f2638e8fb3d019177ebfeb382f540R25

If you still see something wrong, would you mind opening a dedicated issue and provide more details?

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