-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix for incorrect metric causing high cardinality #192
Conversation
src/tracking.js
Outdated
@@ -203,7 +188,7 @@ export function setupLogger() { | |||
dimensions: { | |||
components: getComponents().join(","), | |||
integrationSource, | |||
isPayPalDomain: isLoadedInFrame, | |||
isPayPalDomain: Boolean(isLoadedInFrame), |
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.
I'm not sure if this is still the case but we've seen in the past signalFX dropping boolean values. Should we make sure this is a true/false string or some other string value?
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.
Good observation! I wasn't aware of that - I'll change this to a string
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.
I like the change (after the lint fix) but wanted to propose an optional/additional change in beaver-logger around https://github.com/krakenjs/beaver-logger/blob/87749c48788dfd2525fb9b075ca33a7bf69d3311/src/logger.js#L296-L301
that limits the max cardinality value (truncate and maybe?)console.warn
63bfbe0
to
b3d1ca0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
+ Coverage 73.74% 74.28% +0.54%
==========================================
Files 23 23
Lines 2045 2030 -15
Branches 241 239 -2
==========================================
Hits 1508 1508
+ Misses 527 512 -15
Partials 10 10 ☔ View full report in Codecov by Sentry. |
@jshawl I would love to find a way to validate/limit the cardinality in beaver logger. We could look at the length of the string as a possible indicator, but it is possible to have low cardinality across all of the metrics while also having a long string length. Do you have any thoughts or ideas? |
The
isLoadedInFrame
variable previously wasisPayPalDomain() && window.xprops
. Because of the && operator, the value here would be the value of window.xprops which is a very high cardinality value. We ended up having metrics disabled due to the cardinality of these events so casting to a Boolean should solve for that.One thing I'm not clear on is why we're mapping
isLoadedInFrame
toisPayPalDomain
and not callingisPayPalDomain()
directly, but I'm only trying to solve the cardinality issue on this event.