-
Notifications
You must be signed in to change notification settings - Fork 85
Add OpenTelemetry and basic tracing + dev infra #7163
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile SummaryThis PR adds OpenTelemetry distributed tracing support to Fides, including:
Key Issues Found:
Confidence Score: 3/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
17 files reviewed, 5 comments
| # OpenTelemetry imports are optional - app should work without them | ||
| try: | ||
| from opentelemetry import trace | ||
| from opentelemetry.exporter.otlp.proto.grpc.trace_exporter import OTLPSpanExporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Unused import - OTLPSpanExporter is imported here but local imports are used inside _create_otlp_exporter instead.
| ConsoleSpanExporter, | ||
| SpanExporter, | ||
| ) | ||
| from opentelemetry.sdk.trace.sampling import ParentBasedTraceIdRatio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Unused import - ParentBasedTraceIdRatio is imported but never used (commented-out code on line 110).
| # Determine if we're using gRPC or HTTP based on the endpoint | ||
| endpoint = config.tracing.otlp_endpoint | ||
| protocol = config.tracing.otlp_protocol | ||
| exporter = config.tracing.otlp_exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: exporter variable is assigned but never used.
Remove temporary increased sample rate Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Moved grafana to port 3010 to not conflict with fides-ui Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7163 +/- ##
==========================================
- Coverage 87.17% 85.49% -1.69%
==========================================
Files 534 536 +2
Lines 35312 35515 +203
Branches 4113 4142 +29
==========================================
- Hits 30783 30362 -421
- Misses 3638 4221 +583
- Partials 891 932 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds Open Telemetry along with basic tracing of FastAPI, httpx and sqlalchemy, a convenience tracing module to start new spans, and also adds the following to
nox -s dev: Prometheus (for scraping metrics later), tempo (for collecting OTel traces), memcached (for tempo), and grafana (to view metrics/traces). When your dev setup is running, you can hithttp://localhost:3000and see Grafana. On the left is 'Drilldown > Traces' which will take you to the traces view; down at the bottom is a traces tab that will show you all the traces for the service.