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

Add usage reporting for dashboards as instances #7294

Merged
merged 12 commits into from
Nov 18, 2024

Conversation

sciencewhiz
Copy link
Contributor

Detects dashboards based on network tables client identity
Depends on #7293 to detect SmartDashboard

Detects dashboards based on network tables client identity
Depends on wpilibsuite#7293 to detect SmartDashboard
@sciencewhiz sciencewhiz added attn: NI component: usage reporting Analytics for library usage sent to FIRST on competition fields. labels Oct 27, 2024
} else {
// Only report unknown if there wasn't another dashboard already reported
// (unknown could also be another device)
if (!m_detected) {
Copy link
Member

Choose a reason for hiding this comment

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

This would likely report a coprocessor as an unknown dashboard before a DS laptop connects, right? I presume we're okay with that, just calling it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but if a dashboard connected later, it would override the unknown. I just didn't want the unknown to override something known.

@Gold872
Copy link

Gold872 commented Oct 28, 2024

I'm a big fan of this PR, there's one concerning change that I want to let you know about. Within the next update for Elastic I'm going to change the client identity from "elastic" to "Elastic". To prevent this from causing it to be reported as unknown there should be some sort of case-insensitive check or add || t.connInfo.remote_id.startsWith("Elastic") to allow older versions to still be reported properly.

@sciencewhiz sciencewhiz marked this pull request as ready for review November 3, 2024 03:27
@sciencewhiz sciencewhiz requested review from a team as code owners November 3, 2024 03:27
@PeterJohnson PeterJohnson merged commit 57e1075 into wpilibsuite:main Nov 18, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attn: NI component: usage reporting Analytics for library usage sent to FIRST on competition fields.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants