-
Notifications
You must be signed in to change notification settings - Fork 126
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: Add device last connected metrics #1515
Conversation
fixes edgexfoundry#1470. Add device last connected metrics to Get/Set command. Signed-off-by: Lindsey Cheng <beckysocute@gmail.com>
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.
Use URL escape for device name if configuration.Service.EnableNameFieldEscape
is true
Reference:
https://github.com/edgexfoundry/device-sdk-go/pull/1502/files
https://github.com/edgexfoundry/device-sdk-go/pull/1506/files
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.
async value should also be handled
- Register/Unregister LastConnected on add/remove device cache. - Update LastConnected for async reading Signed-off-by: Lindsey Cheng <beckysocute@gmail.com>
352f41d
to
af69577
Compare
Change's made for |
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.
Nice Job!! Just a few comments.
Add unit tests for new lastConnected methods in deviceCache. Signed-off-by: Lindsey Cheng <beckysocute@gmail.com>
Signed-off-by: Lindsey Cheng <beckysocute@gmail.com>
internal/cache/devices.go
Outdated
registeredName := strings.Replace(lastConnectedPrefix, deviceNameText, deviceName, 1) | ||
|
||
err := metricsManager.Register(registeredName, metric, map[string]string{"device": deviceName}) | ||
err := metricsManager.Register(registeredName, metric, nil) |
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 tag is still needed. This allows for filtering on a dashboard such as Grafana.
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.
Done, tag added back. Thanks.
Add back the device tag to the LastConnected metric. Signed-off-by: Lindsey Cheng <beckysocute@gmail.com>
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.
LGTM
fixes #1470. Add device last connected metrics to Get/Set command.
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/device-sdk-go/blob/main/.github/Contributing.md
PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:
describing the break)Testing Instructions
New Dependency Instructions (If applicable)