-
Notifications
You must be signed in to change notification settings - Fork 142
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
✨ [RUM-4014] DD_LOGS: add handling stack in beforeSend context #2786
✨ [RUM-4014] DD_LOGS: add handling stack in beforeSend context #2786
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2786 +/- ##
=======================================
Coverage 93.63% 93.63%
=======================================
Files 243 244 +1
Lines 7112 7121 +9
Branches 1583 1588 +5
=======================================
+ Hits 6659 6668 +9
Misses 453 453 ☔ View full report in Codecov by Sentry. |
…cro-frontend-service-logs
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
0b4c23a
to
a070bbb
Compare
dcca3e8
to
23ae6a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
♻️ refactor: this file contains function and types that have been moved here untouched to avoid cyclic dependency
packages/logs/src/domain/logger.ts
Outdated
ok(message: string, messageContext?: object, error?: Error) { | ||
this.log(message, messageContext, StatusType.ok, error) | ||
let handlingStack: string | undefined | ||
|
||
if (isAuthorized(StatusType.ok, HandlerType.http, this)) { | ||
handlingStack = createHandlingStack() | ||
} | ||
|
||
this.logImplementation(message, messageContext, StatusType.ok, error, handlingStack) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: Maybe it could be interesting to use .bind
to avoid duplicating so much code? Something like:
class Logger {
constructor(...) {
...
this.ok = logImplementation.bind(this, StatusType.ok)
}
}
This could be a minor breaking change though (ex: if a customer has a class that extends Logger and calls super.ok()
)
Else, I would suggest to always create the handlingstack, as it would reduce a bit the amount of boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always create the handlingstack
Indeed, this would save ~660 Bytes as per my calculations. However generating a stack trace could be expensive, especially if used a lot.
Another solution is to skip 3 frame instead of 2 for logs. Customers can always increase the stack size if needed.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply the prototype trick we talked during the daily. +240B vs +909B before 🎉
Thank you @BenoitZugmeyer for the suggestion
packages/logs/src/domain/networkError/networkErrorCollection.ts
Outdated
Show resolved
Hide resolved
6f225a9
to
383b43a
Compare
/to-staging |
🚂 Branch Integration: starting soon, median merge time is 10m Commit ef8878c0d6 will soon be integrated into staging-24. Use |
…ing-24 Integrated commit sha: ef8878c Co-authored-by: Thomas Lebeau <thomas.lebeau@datadoghq.com>
🚂 Branch Integration: This commit was successfully integrated Commit ef8878c0d6 has been merged into staging-24 in merge commit c724037f68. Check out the triggered pipeline on Gitlab 🦊 |
Motivation
Create a stack trace and expose it in the
beforeSend
'sdomainContext
argument.It allow customers to initialise the browser SDK with something like:
Changes
console.*()
logger.*()
fetch
console errors?xhr
console errors?isAuthorized
to a new file to avoid cyclic dependenciesTesting
I have gone over the contributing documentation.