-
Notifications
You must be signed in to change notification settings - Fork 13
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
Move pe app usage to metrics #39
Move pe app usage to metrics #39
Conversation
For API changes see lf-edge/eve-api#39 Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
proto/info/info.proto
Outdated
@@ -930,8 +930,7 @@ message ZInfoApp { | |||
repeated ZInfoNetwork network = 16; // up/down; allocated IP | |||
repeated string volumeRefs = 17; // volume UUIDs | |||
repeated ZInfoSnapshot snapshots = 18; // optional field, used to send list of snapshots on device | |||
// optional; information about usage of every patchEnvelope assigned to app | |||
repeated ZInfoPatchEnvelopeUsage patchEnvelope = 19; | |||
// deprecated = 19; struct was removed |
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.
How about using
reserved: 19; // deprecated
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.
Will update this and yetus complaints
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.
@eriknordmark do you know if field number 10 in this struct is used? I'd also reserved it
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 reserved field number 10 just in case
proto/metrics/metrics.proto
Outdated
@@ -373,6 +373,21 @@ message appMetric { | |||
repeated appContainerMetric container = 7; | |||
|
|||
AppMemoryMetric appMemory = 8; | |||
|
|||
repeated appPatchEnvelopeMetric patchEnvelope = 9; |
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.
This is under the appMetric. Do we care about reporting any metrics for patch envelopes which are not (yet) associated with any app instances?
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.
Depends on weather we want sidecar container to be an app entity
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 assuming the removed message hasn't yet been used by any controller.
I guess EVE doesn't send the old one so this would just be a compile time issue even if code has been written in some controller?
It does send this info to controller. I have a patch for EVE here |
PatchEnvelope AppInst usage is metric, not info. In order for us to provide consistent updates on patch envelope app usage, we will have to send information about app which can increase network load. In contrast, metrics send could be configured on how often they will be sent Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
f1729fc
to
4705b5f
Compare
For API changes see lf-edge/eve-api#39 Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
For API changes see lf-edge/eve-api#39 Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
For API changes see lf-edge/eve-api#39 Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
PatchEnvelope AppInst usage is metric, not info. In order for us to
provide consistent updates on patch envelope app usage, we will have to
send information about app which can increase network load.
In contrast, metrics send could be configured on how often they will be sent