-
Notifications
You must be signed in to change notification settings - Fork 79
Add option to provide resource based on metric descriptor. #231
Conversation
/cc @yanweiguo |
Codecov Report
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
+ Coverage 73.37% 73.72% +0.34%
==========================================
Files 15 15
Lines 1615 1625 +10
==========================================
+ Hits 1185 1198 +13
+ Misses 352 350 -2
+ Partials 78 77 -1
Continue to review full report at Codecov.
|
metrics.go
Outdated
resource := se.metricRscToMpbRsc(metric.Resource) | ||
var resource *monitoredrespb.MonitoredResource | ||
if get := se.o.ResourceByDescriptor; get != nil { | ||
mr := get(&metric.Descriptor) |
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.
What metric.Descriptor
will be if I register the following:
requestCountM = stats.Int64(
"request_count",
"The number of requests",
stats.UnitDimensionless)
view.Register(
&view.View{
Description: "The number of requests",
Measure: requestCountM,
Aggregation: view.Count(),
TagKeys: append(metrics.CommonRevisionKeys, metrics.PodTagKey, metrics.ContainerTagKey,
metrics.ResponseCodeKey, metrics.ResponseCodeClassKey, metrics.NumTriesKey),
})
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.
metric.Descriptor{
Name: "request_count",
Description: "The number of requests",
Unit: metricdata. UnitDimensionless,
Type: metric.TypeCumulativeInt64,
}
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.
Could we determine resource type by name? Name is more like an ID instead of the description. WDYT?
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.
actually Descriptor.Name = View.Name.
knative does determine resource type from name as per this.
It seems you also need tags, can you confirm?
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.
Ah got it.
The tags are used to set the values of monitored resource labels. For example, project_id
.
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. Just some nits.
Thanks a lot for this quick fix.
stackdriver.go
Outdated
// The ResourceByDescriptor is called to derive monitored resources from | ||
// metric.Descriptor and label map associated with the metric. If any label is | ||
// used for the derived resource then it will be removed the label map. | ||
// Returned label map would then be a subset of the original map. |
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.
Could you describe what the return label maps are used 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.
added the description.
metrics_test.go
Outdated
@@ -559,3 +560,313 @@ func TestMetricsToMonitoringMetrics_fromProtoPoint(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
func TestGetMonitorResource(t *testing.T) { |
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: Should the name be TestResourceByDescriptor
?
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.
changed.
@yanweiguo PTAL. |
LGTM |
@bogdandrutu, @songy23 , @dinooliva can one of you please review this? |
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 except one minor nit.
metrics.go
Outdated
|
||
var rsc *monitoredrespb.MonitoredResource | ||
var mr monitoredresource.Interface | ||
if get := se.o.ResourceByDescriptor; get != 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.
Nit: why not just se.o.ResourceByDescriptor != 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.
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.
@songy23 Thanks for reviewing.
metrics.go
Outdated
|
||
var rsc *monitoredrespb.MonitoredResource | ||
var mr monitoredresource.Interface | ||
if get := se.o.ResourceByDescriptor; get != 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.
done.
fixes #230