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

tracing: make otel dependency optional for rego+topdown #4127

Merged
merged 5 commits into from Dec 14, 2021

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Dec 13, 2021

This follows the same approach as the wasm feature: by default, importers
of

github.com/open-policy-agent/opa/rego
github.com/open-policy-agent/opa/topdown

will not get a transitive dependency on the otel libraries.

In terms of functionality, nothing changes for the server and runtime.


Follow-up to #4029, discussed on that PR.

@srenatus srenatus force-pushed the sr/otel/make-dep-optional branch 2 times, most recently from d215fb0 to 5eda605 Compare December 13, 2021 11:11
@srenatus srenatus marked this pull request as ready for review December 13, 2021 11:14
This follows the same approach as the wasm feature: by default, importers
of

    github.com/open-policy-agent/opa/rego
    github.com/open-policy-agent/opa/topdown

will not get a transitive dependency on the otel libraries.

In terms of functionality, nothing changes for the server and runtime.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
tsandall
tsandall previously approved these changes Dec 13, 2021
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

LGTM. Must be a law of software development that as a project ages it eventually imports every logging library in existence.

tracing = ht
}

func NewTransport(tr http.RoundTripper, opts Options) http.RoundTripper {
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing docstrings on these exported functions. I'm surprised that golangci isn't catching this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm maybe it picks up that it's used by implementing an interface? Interface implementations might not need their own comments...? 🤔

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus srenatus merged commit 3968ad0 into open-policy-agent:main Dec 14, 2021
@srenatus srenatus deleted the sr/otel/make-dep-optional branch December 14, 2021 08:57
yzeng25 pushed a commit to yzeng25/opa that referenced this pull request Dec 14, 2021
…agent#4127)

This follows the same approach as the wasm feature: by default, importers
of

    github.com/open-policy-agent/opa/rego
    github.com/open-policy-agent/opa/topdown

will not get a transitive dependency on the otel libraries.

In terms of functionality, nothing changes for the server and runtime.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
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

Successfully merging this pull request may close these issues.

None yet

2 participants