-
Notifications
You must be signed in to change notification settings - Fork 2
Added string() to metric labels for printing and added const metric strings. #73
base: master
Are you sure you want to change the base?
Conversation
@@ -47,6 +47,11 @@ const ( | |||
CreateFile | |||
) | |||
|
|||
// String returns the ProbeOperation as a string. | |||
func (p ProbeOperation) String() string { | |||
return ProbeOpName[p] |
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.
Nit: if this is the only usage of ProbeOpName, then make it non-exported.
@@ -61,6 +66,11 @@ const ( | |||
APIGetFile | |||
) | |||
|
|||
// String returns the APICall as a string. | |||
func (a APICall) String() string { | |||
return APICallName[a] |
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.
Same as above.
@@ -90,6 +100,10 @@ const ( | |||
AllFilesMissing | |||
) | |||
|
|||
func (e ExitStatus) String() string { | |||
return ExitStatusName[e] |
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.
And here.
@@ -121,6 +135,9 @@ var ( | |||
UnknownFileFound: "unknown_file_found", | |||
AllFilesMissing: "all_files_missing", | |||
} | |||
|
|||
ProbeLatency = "hermes_probe_latency_seconds" |
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.
Maybe ProbeLatencyMetric?
@@ -121,6 +135,9 @@ var ( | |||
UnknownFileFound: "unknown_file_found", | |||
AllFilesMissing: "all_files_missing", | |||
} | |||
|
|||
ProbeLatency = "hermes_probe_latency_seconds" | |||
APILatency = "hermes_api_latency_Seconds" |
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.
Ditto.
@@ -157,7 +174,7 @@ func NewMetrics(conf *probepb.HermesProbeDef, target *probepb.Target) (*Metrics, | |||
m.ProbeOpLatency[op] = make(map[ExitStatus]*metrics.EventMetrics, len(ExitStatusName)) | |||
for e := range ExitStatusName { | |||
m.ProbeOpLatency[op][e] = metrics.NewEventMetrics(time.Now()). | |||
AddMetric("hermes_probe_latency_seconds", probeOpLatDist.Clone()). | |||
AddMetric(ProbeLatency, probeOpLatDist.Clone()). | |||
AddLabel("storage_system", target.GetTargetSystem().String()). |
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.
Make these labels consts too.
@@ -161,10 +161,10 @@ func (mp *Probe) runProbe(ctx context.Context, metricChan chan<- *cpmetrics.Even | |||
exitStatus, err := mp.runProbeForTarget(probeCtx, t) | |||
if err != nil { | |||
mp.logger.Errorf(err.Error()) | |||
t.LatencyMetrics.ProbeOpLatency[metrics.TotalProbeRun][exitStatus].Metric("latency").AddFloat64(time.Now().Sub(start).Seconds()) | |||
t.LatencyMetrics.ProbeOpLatency[metrics.TotalProbeRun][exitStatus].Metric("hermes_probe_latency_seconds").AddFloat64(time.Now().Sub(start).Seconds()) |
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 the consts.
Here and elsewhere.
@@ -161,10 +161,10 @@ func (mp *Probe) runProbe(ctx context.Context, metricChan chan<- *cpmetrics.Even | |||
exitStatus, err := mp.runProbeForTarget(probeCtx, t) | |||
if err != nil { | |||
mp.logger.Errorf(err.Error()) |
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.
Not related to this change, but you should add some context to this error.
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.
Happy to LGTM, assuming you fix all leczb's comments.
No description provided.