Skip to content
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

Add Metric-Specific Labels to googlecloudmonitoringreceiver Metrics enrichment #35711

Closed
navaneeth-c opened this issue Oct 9, 2024 · 6 comments
Assignees

Comments

@navaneeth-c
Copy link

navaneeth-c commented Oct 9, 2024

Component(s)

receiver/googlecloudmonitoring

Is your feature request related to a problem? Please describe.

The Google Cloud Monitoring Receiver currently enriches metrics with resource-level tags but lacks metric-specific labels (e.g., nat_ip for NAT gateway metrics). Without these labels, users miss crucial context directly related to the metric, limiting filtering and grouping capabilities when monitoring specific resources. Including these metric labels would enhance usability by allowing more precise filter, grouping and tracking based on metric-specific attributes.

Describe the solution you'd like

I propose updating the convertGCPTimeSeriesToMetrics function (here) within the googlecloudmonitoringreceiver/receiver.go file to capture and append metric-specific labels alongside resource tags. Specifically, this enhancement would add metric labels from timeSeries.Metric.Labels to the metric attributes, enriching each metric with both resource and metric-specific context.

Suggested Code:

// Add metric-specific labels if they are present 
if len(timeSeries.Metric.Labels) > 0 {
	for k, v := range timeSeries.Metric.Labels {
		resource.Attributes().PutStr(k, fmt.Sprintf("%v", v))
	}
}

This code snippet ensures that metric-specific labels are added only if they are present.

Describe alternatives you've considered

Custom Processor: We considered building a custom processor to add metric labels post-collection. Howeever, this approach requires additional processing steps and does not fully integrate with the Google Cloud Monitoring Receiver.

Additional context

This feature request aims to improve the observability and filtering options for users relying on GoogleCloudMonitoring metrics. By incorporating metric labels, we offer a more complete metric context without adding significant overhead or complexity. If this aligns with the project's goal, I am ready to contribute by implementing this feature in a PR.

@navaneeth-c navaneeth-c added enhancement New feature or request needs triage New item requiring triage labels Oct 9, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@dashpole dashpole removed the needs triage New item requiring triage label Oct 9, 2024
@dashpole dashpole self-assigned this Oct 9, 2024
@dashpole
Copy link
Contributor

dashpole commented Oct 9, 2024

Oh, dang. We are completely ignoring/dropping metric labels:

// Create a new Metric
m := sm.Metrics().AppendEmpty()
// Set metric name, description, and unit
m.SetName(metricDesc.GetName())
m.SetDescription(metricDesc.GetDescription())
m.SetUnit(metricDesc.Unit)
// Convert the TimeSeries to the appropriate metric type
switch timeSeries.GetMetricKind() {
case metric.MetricDescriptor_GAUGE:
mr.metricsBuilder.ConvertGaugeToMetrics(timeSeries, m)
case metric.MetricDescriptor_CUMULATIVE:
mr.metricsBuilder.ConvertSumToMetrics(timeSeries, m)
case metric.MetricDescriptor_DELTA:
mr.metricsBuilder.ConvertDeltaToMetrics(timeSeries, m)
// TODO: Add support for HISTOGRAM
// TODO: Add support for EXPONENTIAL_HISTOGRAM
default:
metricError := fmt.Sprintf("\n Unsupported metric kind: %v\n", timeSeries.GetMetricKind())
mr.logger.Info(metricError)
}
}

@dashpole
Copy link
Contributor

dashpole commented Oct 9, 2024

@abhishek-at-cloudwerx is this something you were planning to do?

@andrewegel
Copy link

Started using this receiver at my organization - Not adding the metric label can cause miss-reporting; You can see this in the metric router.googleapis.com/nat/sent_bytes_count where if the ip_protocol label isn't added, we were getting reported values of 0 when Google showed these as having vlaues; We found that the Zeros were the cases where ip_protocol=1 or ip_protocol=17, but when ip_protocol=6 there were actual reported values.

I don't have enough knowledge of Open Telemetry's internals to know if thats an issue or not, but we're using the prometheusremotewrite exporter, and since all three series without a ip_protocol attribute, the values for ip_protocol=6 were "lost" when values for ip_protocol=1 or ip_protocol=17 were added.

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Dec 16, 2024
andrzej-stencel pushed a commit that referenced this issue Dec 16, 2024
…#35828)

#### Description
Add metric-specific labels inside the googlecloudmonitoringreceiver
component

#### Link to tracking issue

#35711

#### Testing

#### Documentation
@dashpole
Copy link
Contributor

Fixed by ##35828. Unit tests to follow

sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this issue Dec 17, 2024
…open-telemetry#35828)

#### Description
Add metric-specific labels inside the googlecloudmonitoringreceiver
component

#### Link to tracking issue

open-telemetry#35711

#### Testing

#### Documentation
mterhar pushed a commit to mterhar/opentelemetry-collector-contrib that referenced this issue Dec 19, 2024
…open-telemetry#35828)

#### Description
Add metric-specific labels inside the googlecloudmonitoringreceiver
component

#### Link to tracking issue

open-telemetry#35711

#### Testing

#### Documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants