-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(core): Use serializeAttributes for metric attribute serialization
#18582
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
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
9d87fba to
0a298b5
Compare
|
Update: Fixed |
9019898 to
9105768
Compare
| if (!useFallback || (useFallback === 'skip-undefined' && value === undefined)) { | ||
| return; | ||
| } |
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.
So this is a fun one: metrics and logs attribute setting logic already diverged prior to this PR: undefined values were
- stringified to
''in logs - ignored (not sent) in metrics
I therefore had to make the fallback behavior a tri-state flag. true ignores non-conforming values, 'skip-undefined also ignores undefined in addition, false completely ignores non-conforming values.
pre-work for #18160
Use the same attibute serialization logic we already use in logs.
Bundle size impact:
I think the largest positive long-term aspect of this refactor is that we'll re-use this logic more and more going forward (scope attributes on metrics, spansv2, and other telemetry items in the future), so I'd like to have it unified.