-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
feat: manually use identify protocol #6400
Conversation
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6400 +/- ##
============================================
- Coverage 60.14% 60.14% -0.01%
============================================
Files 407 407
Lines 46512 46510 -2
Branches 1551 1551
============================================
- Hits 27975 27972 -3
- Misses 18505 18506 +1
Partials 32 32 |
.identify(evt.detail) | ||
.then((result) => { | ||
const agentVersion = result.agentVersion; | ||
if (agentVersion) { |
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 am curios about the different cases, i.e. when do we not get an error but also an empty agentVersion? and should we add a fallback case like "N/A"
as previously done?
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.
The protobuf sent to us can have a null entry.
I thought about keeping the fallback "N/A" but it didn't seem particularly useful? Happy to add it back if anyone feels different.
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 thought about keeping the fallback "N/A" but it didn't seem particularly useful?
The only case I see where this makes a difference is in the case we error as it would be undefined
vs "N/A"
otherwise, although I don't see that this value is used anywhere other than dumping peer information.
I remember Tuyen did some changes in known client version but that is just for metrics afaik and we already handle the case if agentClient
is not set
const client = peerData?.agentClient ?? ClientKind.Unknown; |
🎉 This PR is included in v1.16.0 🎉 |
Description