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

AWS Resource detector fails with an error if metadata not available #398

Closed
dackroyd opened this issue Oct 13, 2020 · 6 comments · Fixed by #401
Closed

AWS Resource detector fails with an error if metadata not available #398

dackroyd opened this issue Oct 13, 2020 · 6 comments · Fixed by #401
Assignees
Labels
area: detector Related to a detector package bug Something isn't working
Milestone

Comments

@dackroyd
Copy link
Contributor

If the instance metadata is not available, no resource and an unavailable EC2 client error will be returned

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/7e7d779/detectors/aws/aws.go#L42-L45

client := ec2metadata.New(session)
if !client.Available() {
	return nil, errors.New("unavailable EC2 client")
}

This is counter to the documented behaviour for resources which should be:

Note the failure to detect any resource information MUST NOT be considered an error, whereas an error that occurs during an attempt to detect resource information SHOULD be considered an error.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/sdk.md#detecting-resource-information-from-the-environment

A similar issue was raised (and fixed) with the ENV var detection in the main OTel library: open-telemetry/opentelemetry-go#1114 / open-telemetry/opentelemetry-go#1170

The GCP resource in this contrib library appears to implement detection correctly, returning no error:

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/7e7d779/detectors/gcp/gce.go#L38-L40
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/master/detectors/gcp/gce.go#L102-L104

if !metadata.OnGCE() {
	return nil, nil
}
if _, undefined := err.(metadata.NotDefinedError); undefined {
	return false
}
@MrAlias MrAlias added area: detector Related to a detector package bug Something isn't working priority:p2 labels Oct 13, 2020
@MrAlias MrAlias added this to the RC1 milestone Oct 13, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Oct 13, 2020

Instead of

if _, undefined := err.(metadata.NotDefinedError); undefined {
	return false
}

I would use the new errors package functionality:

if errors.Is(err, metadata.NotDefinedError)
	return false
}

Otherwise, this looks like a good fix. Thanks for opening it.

@dackroyd
Copy link
Contributor Author

dackroyd commented Oct 13, 2020

To clarify, the code snippets I included are the existing code for the aws and gcp detectors: none of that was suggested fix. The err.(metadata.NotDefinedError) is the current code in the gcp detector

@dackroyd
Copy link
Contributor Author

As for a suggested fix, this might be appropriate:

client := ec2metadata.New(session)
if !client.Available() {
	return nil, nil
}

@MrAlias
Copy link
Contributor

MrAlias commented Oct 13, 2020

Ah, my mistake @dackroyd!

Would you be willing to put together a PR with this?

@dackroyd
Copy link
Contributor Author

I'd be happy to push that change 👍

@MrAlias
Copy link
Contributor

MrAlias commented Oct 14, 2020

I'd be happy to push that change

Awesome, thanks for the help! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: detector Related to a detector package bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants