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.
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.
metric.Descriptor{
Name: "request_count",
Description: "The number of requests",
Unit: metricdata. UnitDimensionless,
Type: metric.TypeCumulativeInt64,
}There was a problem hiding this comment.
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.
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.
Ah got it.
The tags are used to set the values of monitored resource labels. For example, project_id.
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.
Could you describe what the return label maps are used for?
There was a problem hiding this comment.
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.
nit: Should the name be TestResourceByDescriptor?
|
@yanweiguo PTAL. |
|
LGTM |
|
@bogdandrutu, @songy23 , @dinooliva can one of you please review this? |
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.
Nit: why not just se.o.ResourceByDescriptor != nil?
metrics.go
Outdated
|
|
||
| var rsc *monitoredrespb.MonitoredResource | ||
| var mr monitoredresource.Interface | ||
| if get := se.o.ResourceByDescriptor; get != nil { |
fixes #230