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

GMP metric integration test is broken #584

Closed
dashpole opened this issue Feb 7, 2023 · 7 comments · Fixed by #585
Closed

GMP metric integration test is broken #584

dashpole opened this issue Feb 7, 2023 · 7 comments · Fixed by #585
Assignees

Comments

@dashpole
Copy link
Contributor

dashpole commented Feb 7, 2023

As part of #583, I found that the GMP tests are not passing when run against a real GCM endpoint.

Failure:

    metrics_integration_test.go:72: 
        	Error Trace:	/workspace/exporter/collector/integrationtest/metrics_integration_test.go:72
        	Error:      	Received unexpected error:
        	            	failed to export time series to GCM: rpc error: code = InvalidArgument desc = One or more TimeSeries could not be written: Request was missing field timeSeries[21].points[0].interval.endTime: The end time of the interval is required.; Request was missing field timeSeries[1].points[0].interval.endTime: The end time of the interval is required.
        	Test:       	TestIntegrationCollectorLogs/Google_Managed_Prometheus
@damemi
Copy link
Member

damemi commented Feb 7, 2023

I'll take a look into this, thanks @dashpole

@damemi
Copy link
Member

damemi commented Feb 7, 2023

At first I assumed I just needed to add a timestamp to the new metrics, but it's already there.

This is set by the exporter calling timestamppb.New(point.Timestamp().AsTime()). I assume this just defaults to the zero time when it's not set. But, I'm wondering if this is working right for any of our points. For example, this timestamp is Fri Apr 08 2022 18:45:16 GMT+0000 but comes out as 1970-01-01T00:00:00Z. The rest of the timestamps in that expect fixture are also the zero time.

That still doesn't explain why this is failing for just these new metrics, so there's obviously something wrong with them in particular. But I thought this was interesting too.

@dashpole
Copy link
Contributor Author

dashpole commented Feb 7, 2023

Are timestamps being set for target_info and scope_info? That was the ones I assumed it impacted

@damemi
Copy link
Member

damemi commented Feb 7, 2023

@dashpole it is in the expect fixture:

"metric": {
"type": "prometheus.googleapis.com/target_info/gauge",
"labels": {
"cloud_platform": "gcp_kubernetes_engine",
"http_scheme": "http",
"k8s_container_name": "rabbitmq",
"k8s_node_name": "10.92.5.2",
"k8s_pod_name": "rabbitmq-server-0",
"net_host_ip": "10.92.5.2",
"net_host_port": "15692"
}
},
"resource": {
"type": "prometheus_target",
"labels": {
"cluster": "rabbitmq-test-dev",
"instance": "10.92.5.2:15692",
"job": "demo",
"location": "us-central1-c",
"namespace": "default"
}
},
"metricKind": "GAUGE",
"valueType": "INT64",
"points": [
{
"interval": {
"endTime": "1970-01-01T00:00:00Z"
},
"value": {
"int64Value": "1"
}
(sorry think I messed up the permalink in my other comment)

@dashpole
Copy link
Contributor Author

dashpole commented Feb 7, 2023

I removed the metrics in question from the fixture in the previous PR: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/583/files#diff-e2bf93fa07e3c8d6067f0b4847f1c665899433fb5797f4d81a54d969d2664aecL60

It isn't clear to me if the expectation's timestamp is overridden by the test fixture sanitization or if it is actually set beforehand. Maybe when we override timestamps in the test, we should make they are not zero first, or output an error?

@damemi
Copy link
Member

damemi commented Feb 7, 2023

Ah right, when we record fixtures we normalize them with an empty timestamp. That's probably why they were already in the expect fixture for target_info. It's supposed to only do it if the end time is nil, so I'm not sure how it got set at some point. But this is something for me to look at

@dashpole
Copy link
Contributor Author

dashpole commented Feb 7, 2023

Maybe its non-nil, but set to zero?

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

Successfully merging a pull request may close this issue.

2 participants