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

feat: add OpenTelemetry instrumentator middleware #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Pushkarm029
Copy link
Collaborator

@Pushkarm029 Pushkarm029 commented Sep 2, 2024

Description

This PR introduces a middleware component for integrating OpenTelemetry instrumentation into a Starlette application. The OpenTelemetryInstrumentor class facilitates the setup of both tracing and metrics collection by configuring the necessary OpenTelemetry providers and exporters.

Key Features:

  • Tracing: Configures a global TracerProvider with BatchSpanProcessor and OTLPSpanExporter to collect and export trace data.
  • Metrics: Sets up a global MeterProvider with PeriodicExportingMetricReader and OTLPMetricExporter for collecting and exporting metric data.
  • Integration: Utilizes StarletteInstrumentor to instrument the Starlette application with OpenTelemetry, enabling automatic collection of tracing and metrics data.
Exception Handling:
  • Raises a ValueError if the provided application instance is not of type Starlette.
  • Raises a RuntimeError if any errors occur during the setup of tracing or metrics providers.

Checklist

  • My code follows the contributing guidelines
    of this project, including, in particular, with regard to any style guidelines
  • The title of my PR complies with the
    Conventional Commits specification; in particular, it clearly
    indicates that a change is a breaking change
  • I acknowledge that all my commits will be squashed into a single commit,
    using the PR title as the commit message
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have updated the user-facing documentation to describe any new or
    changed behavior
  • I have added type annotations for all function/class/method interfaces
    or updated existing ones (only for Python, TypeScript, etc.)
  • I have provided appropriate documentation
    (Google-style Python docstrings) for all
    packages/modules/functions/classes/methods or updated existing ones
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature
    works
  • New and existing unit tests pass locally with my changes
  • I have not reduced the existing code coverage

Comments

  • Currently, I am confused about how we will integrate this middleware with the rest of the services. Once I understand this, I will be able to add tests.@uniqueg

Summary by Sourcery

Integrate OpenTelemetry instrumentation into a Starlette application by adding a middleware component. This includes setting up tracing and metrics collection using OpenTelemetry providers and exporters, and handling exceptions during the setup process.

New Features:

  • Introduce a middleware component for integrating OpenTelemetry instrumentation into a Starlette application, enabling automatic collection of tracing and metrics data.

Enhancements:

  • Configure a global TracerProvider with BatchSpanProcessor and OTLPSpanExporter for trace data collection and export.
  • Set up a global MeterProvider with PeriodicExportingMetricReader and OTLPMetricExporter for metric data collection and export.

Documentation:

  • Add documentation for the OpenTelemetryInstrumentor class and its methods, detailing the setup process for tracing and metrics.

Signed-off-by: pushkarm029 <[email protected]>
@Pushkarm029 Pushkarm029 marked this pull request as ready for review September 2, 2024 12:04
Copy link

sourcery-ai bot commented Sep 2, 2024

Reviewer's Guide by Sourcery

This pull request introduces an OpenTelemetryInstrumentor middleware for Starlette applications, enabling tracing and metrics collection using OpenTelemetry. The implementation includes setting up TracerProvider and MeterProvider with appropriate exporters, and integrates with the Starlette application using StarletteInstrumentor.

File-Level Changes

Change Details Files
Implement OpenTelemetryInstrumentor middleware
  • Create OpenTelemetryInstrumentor class with static method instrument_app
  • Set up TracerProvider with BatchSpanProcessor and OTLPSpanExporter
  • Configure MeterProvider with PeriodicExportingMetricReader and OTLPMetricExporter
  • Use StarletteInstrumentor to instrument the Starlette application
  • Implement error handling for invalid app type and runtime errors
cloud_telemetry/middleware.py
Add initialization function for middleware in main entry point
  • Create init_middleware function to instrument Starlette app
  • Import OpenTelemetryInstrumentor and Starlette
  • Implement error handling for instrumentation failures
cloud_telemetry/main.py

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Pushkarm029 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The PR looks good overall, but it's important to add unit tests to verify the functionality of the new OpenTelemetryInstrumentor and init_middleware function. This will ensure the instrumentation works as expected and help prevent regressions in the future.
Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

cloud_telemetry/main.py Outdated Show resolved Hide resolved
cloud_telemetry/main.py Outdated Show resolved Hide resolved
cloud_telemetry/middleware.py Outdated Show resolved Hide resolved
cloud_telemetry/middleware.py Show resolved Hide resolved
Signed-off-by: pushkarm029 <[email protected]>
README.md Show resolved Hide resolved
cloud_telemetry/main.py Outdated Show resolved Hide resolved
class OpenTelemetryInstrumentor:
"""Instruments a Starlette application with OpenTelemetry."""

@staticmethod
Copy link

Choose a reason for hiding this comment

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

Any reason for staticmethod specifically?

I think this ties us down from making custom choices, I think we would might want these methods to interact with other data and memebers, for example lets say you want to tracing and metric boolean to be defined at class level and then set the dafault from class member, this way user can have more flexibility when envoking these methods, unless ofc you are aiming for some desing pattern like factory patter or singleton patter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice, I have pushed some changes, please have a look.

@JaeAeich JaeAeich changed the base branch from dev to main September 3, 2024 07:32
@uniqueg uniqueg changed the title INIT Opentelemetry Instrumentator as a starlette middleware feat: add OpenTelemetry instrumentator middleware Sep 3, 2024
Signed-off-by: pushkarm029 <[email protected]>
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7037d43) to head (a425fb9).

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #5   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines            8         8           
=========================================
  Hits             8         8           
Flag Coverage Δ
test_integration 100.00% <ø> (ø)
test_unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

2 participants