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

feat: return errors from resource.New #59

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

dstrelau
Copy link
Member

@dstrelau dstrelau commented Jul 31, 2023

Which problem is this PR solving?

When using detectors with mis-matched Schema URLs, resource.New will return an error. Previously, this was dropped and consequently it looked like configuration worked but many resource attributes would be missing in the resulting telemetry.

With the recent addition of #48, this error is much easier to hit. For example, the detectors built into opentelemetry-go import semconv v1.17.0 but this project pulls in v1.18.0. This means that adding something like otelconfig.WithResourceOption(resource.WithHost()) results in a mis-configured OTel and no error to warn you what happened.

This is all … very bad. This PR is really the bare minimum that needs to be done here. That is, it at least causes a runtime error so that you can tell what's going on — but it doesn't solve the problem that you fundamentally cannot use this library with any other library that doesn't happen to use semconv v1.18.0. There are a number of open issues in OTel and adjacent repositories regarding this problem, which probably warrant further investigation into a long-term sustainable solution.

Short description of the changes

Return the err from resource.New up through the stack. Update tests to expect this.

⚠️ I guess technically this could be considered a backwards incompatible change, in that folks may have configurations that are "working" today but will throw an error after this change. I put that in scare quotes though because it's not really working — it's just not throwing an error. I feel like actually returning an appropriate error here and letting folks know their configuration is likely not working as expected is the right choice.

How to verify that this has the expected result

There's a new test that specifically uses a detector with a different schema URL.

@dstrelau dstrelau requested a review from a team July 31, 2023 16:46
When using detectors with mis-matched Schema URLs, resource.New
will return an error. Previously, this was dropped and consequently it
looked like configuration worked but many resource attributes would be
missing in the resulting telemetry.
@dstrelau dstrelau changed the title return errors from resource.New [feat] return errors from resource.New Jul 31, 2023
@dstrelau dstrelau changed the title [feat] return errors from resource.New feat: return errors from resource.New Jul 31, 2023
@cartermp
Copy link
Member

Ahhhh this 'ole bucket of fun. I ended up documenting things to fail loudly because of this:

When using detectors with mis-matched Schema URLs, resource.New will return an error.

In the docs: https://opentelemetry.io/docs/instrumentation/go/manual/#setup

func newTraceProvider(exp sdktrace.SpanExporter) *sdktrace.TracerProvider {
	// Ensure default SDK resources and the required service name are set.
	r, err := resource.Merge(
		resource.Default(),
		resource.NewWithAttributes(
			semconv.SchemaURL,
			semconv.ServiceName("ExampleService"),
		),
	)

	if err != nil {
		panic(err)
	}

	return sdktrace.NewTracerProvider(
		sdktrace.WithBatcher(exp),
		sdktrace.WithResource(r),
	)
}

TBH I can't think of a better way to handle this, since there's no other way to actually get the right instrumentation.

@dstrelau
Copy link
Member Author

It does look like this is on the minds of the OTel folks: open-telemetry/opentelemetry-go#4364

@MikeGoldsmith MikeGoldsmith self-assigned this Aug 1, 2023
@MikeGoldsmith MikeGoldsmith added status: oncall Flagged for awareness from Honeycomb Telemetry Oncall type: enhancement New feature or request labels Aug 3, 2023
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! I'll follow this up with some version updates and a couple more tests to hopefully make this a bit less painful to manage.

@JamieDanielson JamieDanielson merged commit 7129f53 into honeycombio:main Aug 14, 2023
3 checks passed
vreynolds pushed a commit that referenced this pull request Aug 15, 2023
…60)

## Which problem is this PR solving?

- it's easy to mismatch semantic convention versions (see notes in #59 )

## Short description of the changes

- Drop our version of semantic conventions to 1.17 to match those used
in the imported otel-go package
- Update the README to clarify versions in use (note old semconv listed
because that's in current available release)
- Update releasing notes to remember to adjust README if versions
change, and some other cleanup for consistency to other repos
- Add simple unit test for checking semantic convention version against
otel-go imported package semantic convention
- Add unit tests for resource detector from core (WithHost) and contrib
(lambda) to confirm no schema error

## How to verify that this has the expected result

Using a detector on the same version should work. These tests should
fail if you change the semantic convention version back to 1.18.0 in the
imports.
@JamieDanielson JamieDanielson added the breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. status: oncall Flagged for awareness from Honeycomb Telemetry Oncall type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants