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

Intermittent extra space characters in prometheus.Metric String() calls after upgrade from v0.3.0 to v0.6.0 #83

Closed
blkperl opened this issue Mar 4, 2024 · 2 comments

Comments

@blkperl
Copy link

blkperl commented Mar 4, 2024

The argo workflow projects recently updated the following dependencies in commit argoproj/argo-workflows@33ad82f. Afterwards, we started seeing intermittent test failures where sometimes extra spaces would be present in the String() response.

--- FAIL: TestCounterMetric (0.01s)
    operator_metrics_test.go:224: 
        	Error Trace:	/home/runner/work/argo-workflows/argo-workflows/workflow/controller/operator_metrics_test.go:224
        	Error:      	"label:{name:\"name\"  value:\"flakey\"}  counter:{value:1  created_timestamp:{seconds:1709341821  nanos:61663999}}" does not contain "label:{name:\"name\" value:\"flakey\"} counter:{value:1"
        	Test:       	TestCounterMetric

The following versions were changed in the commit. I've also tested the v1.19.0 of client_golang and it fails as well.

  • github.com/prometheus/client_golang v1.16.0 => v1.18.0
  • github.com/prometheus/client_model v0.3.0 => v0.6.0
  • github.com/prometheus/common v0.42.0 => v0.48.0

Here is the relevant test code that is failing

func TestCounterMetric(t *testing.T) {

	// .... additional setup ...
	woc = newWorkflowOperationCtx(woc.wf, controller)
	woc.operate(ctx)

        // .... additional assertions ....
	metricTotalCounterString, err := getMetricStringValue(metricTotalCounter)
	assert.NoError(t, err)
	assert.Contains(t, metricTotalCounterString, `label:{name:"name" value:"flakey"} counter:{value:1`)
}

func getMetricStringValue(metric prometheus.Metric) (string, error) {
	metricString := &dto.Metric{}
	err := metric.Write(metricString)
	if err != nil {
		return "", err
	}
	return metricString.String(), nil
}

I've opened argoproj/argo-workflows#12737 as a workaround for now.

@SuperQ
Copy link
Member

SuperQ commented Mar 5, 2024

This is sadly an intentionally breaking change by the upstream protobuf library. They intentionally inject random spaces into the string output in order to prevent people from doing string comparisons in tests.

The upstream considers this working as intended. golang/protobuf#1121

We ended up having to change all our tests to marshal string to proto and compare protos.

See prometheus/client_golang#1323

@SuperQ SuperQ closed this as completed Mar 5, 2024
@agilgur5
Copy link

agilgur5 commented Mar 5, 2024

Oh, did not expect that, thanks for the explanation and links/references @SuperQ!

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

No branches or pull requests

3 participants