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

Add tracing to bundle/discovery download #5973

Conversation

mjungsbluth
Copy link
Contributor

@mjungsbluth mjungsbluth commented Jun 5, 2023

Why the changes in this PR are needed?

Addresses #5967, adds the ability to configure distributed tracing options to plugin manager and rest client.

This allows using an existing distributed tracing infrastructure that is used for the http built-in for bundle downloads without relying in OpenTelemetry.

What are the changes in this PR?

  • Adds tracing.Options to the relevant places to configure bundle download tracing

ashutosh-narkar
ashutosh-narkar previously approved these changes Jun 6, 2023
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @mjungsbluth! LGTM.

@@ -365,6 +367,13 @@ func WithTracerProvider(tracerProvider *trace.TracerProvider) func(*Manager) {
}
}

// DistributedTracingOpts sets the options to be used by distributed tracing.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: DistributedTracingOpts -> WithDistributedTracingOpts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok missed that one(the convention differs on the place where it is used. Will change it for consistency

}

if exp, act := 1, mock.called; exp != act {
t.Errorf("calls to NewTransported: expected %d, got %d", exp, act)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Do you mean NewTransport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will change it at the place where it was copied from as well 😀

Signed-off-by: Magnus Jungsbluth <magnus@jungsbluth.de>
@ashutosh-narkar ashutosh-narkar merged commit 22619d2 into open-policy-agent:main Jun 6, 2023
27 checks passed
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