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

Enable tracing for discovery plugin #7299

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

mjungsbluth
Copy link
Contributor

This PR fixes an oversight introduced with #5973 that actually did not enable tracing for the discovery plugin.

This change centralizes the service options that are used for rest client creation on the plugin manager.

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Looks good to me, but a test to verify this function works as expected would be good. In particular if it passed review once already without actually working :)

@mjungsbluth
Copy link
Contributor Author

Looks good to me, but a test to verify this function works as expected would be good. In particular if it passed review once already without actually working :)

Yes, fully agree. Are there already tests that prove tracing works? I might otherwise be a bit biased to proving it works as a library user (we have tests in Skipper for tracing but they also did not test discovery independently)

@ashutosh-narkar
Copy link
Member

Are there already tests that prove tracing works?

There is one here.

@mjungsbluth mjungsbluth force-pushed the discovery_tracing branch 2 times, most recently from 5e5cc25 to 06f9ab5 Compare January 24, 2025 16:14
@mjungsbluth
Copy link
Contributor Author

Are there already tests that prove tracing works?

There is one here.

Ok, so I included another test. It currently validates that tracing for discovery and bundle download works. I can add the same for decision logs and status if needed.

Two callouts:

  • since the tests use the unaltered runtime, this will to my understanding enable this for all OPA users if tracing is enabled. I.e. there will be more spans than before
  • The operation name of the spans is generally unaltered. At least in our use of OPA we separate control plane traffic from data plane traffic in the operation name so that we can track error percentiles and so one independently. Currently is is the default otel HTTP GET which is mildly helpful. I have no opinion on this how OPA should behave, a change later will break the interface though and potentially impact derived metrics and alerts.

Copy link

netlify bot commented Jan 27, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 270735e
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/6797b4278ef2c700084f8519
😎 Deploy Preview https://deploy-preview-7299--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mjungsbluth mjungsbluth force-pushed the discovery_tracing branch 2 times, most recently from 09eaad1 to 73a213a Compare January 27, 2025 16:42
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

netlify bot commented Jan 28, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit c424119
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/67a3b0d63203db00088a737c
😎 Deploy Preview https://deploy-preview-7299--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mjungsbluth mjungsbluth force-pushed the discovery_tracing branch 2 times, most recently from 043b885 to 949358e Compare February 4, 2025 16:08
@mjungsbluth
Copy link
Contributor Author

I changed all triggermodes to manual, this made the tests and the race detection stable

@mjungsbluth
Copy link
Contributor Author

@anderseknert could you have a final look (the only thing that changed since last time is manually triggering the plugins to make the tests stable)?

@anderseknert
Copy link
Member

@mjungsbluth just did! Have rebased and will merge once the tests have run.

@anderseknert anderseknert merged commit 3591a08 into open-policy-agent:main Feb 5, 2025
28 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.

3 participants