[AlibabaCloudLogService] sanitize labels for metrics #3454
Conversation
There was a problem hiding this comment.
The prometheus exporter sanitizes the metric name and label key and that the AlibabaCloudLogService export only sanitizes the label key, Does the exporter need to concern it too?
Metric names have already sanitized in formatMetricName : https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/alibabacloudlogserviceexporter/metricsdata_to_logservice.go#L139 |
| func (kv *KeyValues) Sort() { sort.Sort(kv) } | ||
|
|
||
| func (kv *KeyValues) Sort() { | ||
| // sanitize label keys before sort |
There was a problem hiding this comment.
Shouldn't sanitization happen on insertion, not sorting? It seems mysterious for a sort method to modify the data itself.
There was a problem hiding this comment.
done, sanitize keys when insert
| @@ -64,6 +68,7 @@ func (kv *KeyValues) Replace(key, value string) { | |||
|
|
|||
| func (kv *KeyValues) AppendMap(mapVal map[string]string) { | |||
| for key, value := range mapVal { | |||
| key = sanitize(key) | |||
There was a problem hiding this comment.
It looks like you can replace these 5 lines with calling the Append function
There was a problem hiding this comment.
Yes, AppendMap can call Append directly
| @@ -51,9 +51,13 @@ func (kv *KeyValues) Swap(i, j int) { | |||
| kv.keyValues[i], kv.keyValues[j] = kv.keyValues[j], kv.keyValues[i] | |||
| } | |||
| func (kv *KeyValues) Less(i, j int) bool { return kv.keyValues[i].Key < kv.keyValues[j].Key } | |||
| func (kv *KeyValues) Sort() { sort.Sort(kv) } | |||
|
|
|||
| func (kv *KeyValues) Sort() { | |||
There was a problem hiding this comment.
Can we add unit tests for this file to demonstrate the sanitization?
Description:
OpenTelemetry metric's labels contains '.' or others none Letter or Digit characters, the alibabacloudlogserviceexporter do not replace these characters, so promQL will cause errors like "promql error: 1:33: parse error: unexpected character inside braces: '.'"
So we sanitize labels before send to sls, the code is almost copied from : https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/prometheusexporter/sanitize.go
Link to tracking Issue:
#3429
Testing:
Documentation: