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-1097] revamp configuration - logs #1217

Merged
merged 7 commits into from
Dec 16, 2021

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Dec 14, 2021

Motivation

Followup of #1216 . This PR focuses on the logs part.

Changes

  • Move init configuration typings into a new logs/src/domain/configuration module
  • Introduce a new LogsConfiguration to be used internally in the Logs SDK
  • Validate and build the configuration in init(), by using the previously introduced validateAndBuildConfiguration function from core
  • Add forwardErrorToLogs to LogsConfiguration, to avoid using InitConfiguration outside of init()

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner December 14, 2021 15:59
Comment on lines -80 to -84
LOGS.init({ clientToken: 'yes', sampleRate: 'foo' as any })
expect(displaySpy).toHaveBeenCalledTimes(1)

LOGS.init({ clientToken: 'yes', sampleRate: 200 })
expect(displaySpy).toHaveBeenCalledTimes(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a test covering the 'foo' case in core but it does not seem that we still have one about '200'.
what about adding it to keep this case?

@codecov-commenter
Copy link

Codecov Report

Merging #1217 (283cd91) into main (2882fa6) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1217      +/-   ##
==========================================
- Coverage   89.08%   89.00%   -0.09%     
==========================================
  Files         100      101       +1     
  Lines        4317     4321       +4     
  Branches      985      986       +1     
==========================================
  Hits         3846     3846              
- Misses        471      475       +4     
Impacted Files Coverage Δ
packages/logs/src/boot/logsPublicApi.ts 100.00% <100.00%> (ø)
packages/logs/src/boot/startLogs.ts 86.66% <100.00%> (-6.96%) ⬇️
packages/logs/src/domain/configuration.ts 100.00% <100.00%> (ø)
packages/logs/src/domain/logsSessionManager.ts 100.00% <100.00%> (ø)
packages/logs/src/domain/trackNetworkError.ts 88.88% <100.00%> (-3.71%) ⬇️
packages/logs/src/transport/startLoggerBatch.ts 81.81% <100.00%> (ø)
...ges/core/src/domain/configuration/configuration.ts 95.23% <0.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 2882fa6...283cd91. Read the comment docs.

@BenoitZugmeyer BenoitZugmeyer merged commit b6d4630 into main Dec 16, 2021
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/revamp-configuration-logs branch December 16, 2021 15:30
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