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

Use tracer and meter test helpers from the OTEL repository instead of the internal ones #414

Merged

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Oct 19, 2020

For some time, the test helpers have existed in parallel in both otel and contrib repositories. Although helpers were functionally very similar in both repositories, not all helpers have been exported from the main otel repository, which resulted in some duplication of helper methods.

This is no longer the case, as helpers for both tracer and meter are now "stabilized" and exported in the main otel repository (go.opentelemetry.io/otel/api/trace/tracetest and go.opentelemetry.io/otel/api/metric/metrictest as of current version 0.13.0). This makes it possible to replace internal helpers in contrib by the ones exported from otel repo and subsequently to remove internal helpers, resolving in cleaner and slimmer codebase.

Changes include:

  • Replacing internal helpers by external ones in instrumentation modules + related test changes
  • Replacing internal helpers by external ones in propagators modules + related test changes
  • Removal of internal helper packages
  • go mod related changes in affected modules

Resolves #385 and effectively resolves #31, #43 as well

@matej-g
Copy link
Contributor Author

matej-g commented Oct 19, 2020

FYI, looks like from next version, the test helper packages in otel repo have been moved by open-telemetry/opentelemetry-go#1229 and open-telemetry/opentelemetry-go#1252, so the import paths should be changed to go.opentelemetry.io/otel/oteltest before the next release.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks solid 👍

Thanks for digging into this!

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias MrAlias added priority:p2 area: testing Related to package testing labels Oct 27, 2020
@MrAlias MrAlias added this to the RC1 milestone Oct 27, 2020
@MrAlias MrAlias added dependencies Pull requests that update a dependency file and removed dependencies Pull requests that update a dependency file labels Oct 27, 2020
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

I'm overall in favor of this, but the Event addition in #413 makes me think perhaps we should ensure that we have fully-working helper implementations before relying on the main repo. Otherwise we may find ourselves in a situation where multiple releases need to happen in order to properly test an addition in this repo.

@matej-g
Copy link
Contributor Author

matej-g commented Oct 29, 2020

I'm overall in favor of this, but the Event addition in #413 makes me think perhaps we should ensure that we have fully-working helper implementations before relying on the main repo. Otherwise we may find ourselves in a situation where multiple releases need to happen in order to properly test an addition in this repo.

I guess Events are in the oteltest package already (https://github.com/MrAlias/opentelemetry-go/commits/master/oteltest/event.go). Are we missing something else in the main repo?

@MrAlias
Copy link
Contributor

MrAlias commented Nov 19, 2020

@Aneurysm9 are you good with merging this and possibly opening an issue to address the concerns you raised?

@Aneurysm9
Copy link
Member

Yup, I'm good. It looks like the main repo has more in the oteltest package than the subset here, so we should be good to use that package.

@MrAlias MrAlias merged commit b83f931 into open-telemetry:master Nov 20, 2020
@MrAlias MrAlias mentioned this pull request Nov 20, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testing Related to package testing
Projects
None yet
4 participants