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

Compatible aggregators should not error instrument creation #3240

Closed
MrAlias opened this issue Sep 28, 2022 · 0 comments · Fixed by #3251
Closed

Compatible aggregators should not error instrument creation #3240

MrAlias opened this issue Sep 28, 2022 · 0 comments · Fixed by #3251
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 28, 2022

The OTel spec says:

  • For each View, if the Instrument could match the instrument selection criteria:
    • Try to apply the View configuration. If applying the View results in conflicting metric identities the implementation SHOULD apply the View and emit a warning. If it is not possible to apply the View without producing semantic errors (e.g. the View sets an asynchronous instrument to use the Explicit bucket histogram aggregation) the implementation SHOULD emit a warning and proceed as if the View did not exist.

If a view translates an instrument into the exact same as another instrument the same aggregators from the already registered instrument should be updated and pulled from.

Currently, we do not check compatibility:

if _, ok := p.aggregations[scope][inst]; ok {
return fmt.Errorf("%w: name %s, scope: %s", errAlreadyRegistered, name, scope)
}

Instead, we just return an error saying the instrument was already registered.

To make matters worse, with the error we return an internal.Aggregator that will never be pulled from:

aggs = append(aggs, agg)
seen[id] = struct{}{}
err = i.pipeline.addAggregator(inst.Scope, inst.Name, inst.Description, instUnit, agg)
if err != nil {
errs.append(err)
}
}
// TODO(#3224): handle when no views match. Default should be reader
// aggregation returned.
return aggs, errs.errorOrNil()

Environment

  • opentelemetry-go version: aca054b

Steps To Reproduce

Change this line to check for NoError

Run the tests

$ go test .
--- FAIL: TestPipelineRegistryCreateAggregatorsDuplicateErrors (0.00s)
    pipeline_registry_test.go:415:
        	Error Trace:	pipeline_registry_test.go:415
        	Error:      	Received unexpected error:
        	            	%!w(<nil>): could not create all aggregators: instrument already registered: name foo, scope: {  }
        	Test:       	TestPipelineRegistryCreateAggregatorsDuplicateErrors
FAIL
FAIL	go.opentelemetry.io/otel/sdk/metric	0.010s
FAIL

Expected behavior

Test should pass with that modification and the returned internal.Aggregator should update the existing registered on.e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant