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

Prototype @instrument with OTEL #1693

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

Conversation

sfc-gh-gtokernliang
Copy link
Contributor

@sfc-gh-gtokernliang sfc-gh-gtokernliang commented Dec 12, 2024

Description

Create an @Instrument decorator that emits the OTEL spans to the created Event table

Other details good to know for developers

N/A

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • New Tests
  • This change includes re-generated golden test results
  • This change requires a documentation update

Important

Adds @instrument decorator for OpenTelemetry tracing, enhances Snowflake support in SQLAlchemy, and includes tests for new functionality.

  • New Feature:
    • Adds @instrument decorator in instrument.py for OpenTelemetry tracing, emitting OTEL spans.
    • Validates attributes, including TRULENS_SELECTOR_NAME from semantic.py.
    • Handles exceptions by setting span status to STATUS_CODE_ERROR.
  • Database:
    • Adds patch_insert function in sqlalchemy.py to support Snowflake OBJECT in SQLAlchemy ORM.
    • Adds get_db_dialect() method in SQLAlchemyDB class in sqlalchemy.py.
  • Export:
    • Updates export() in exporter.py to correctly handle parent_id for spans.
    • Adds event_id to _construct_event() in exporter.py.
  • Tests:
    • Adds test_otel_instrument.py to test @instrument decorator functionality.
    • Includes golden file test_otel_instrument__test_deterministic_app_id.csv for expected results.
    • Updates WithJSONTestCase in test.py to handle .csv and .parquet formats.

This description was created by Ellipsis for a1c461c. It will automatically update as commits are pushed.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@sfc-gh-gtokernliang sfc-gh-gtokernliang changed the title [WIP] Instrument Prototype @instrument with OTEL Dec 12, 2024
@sfc-gh-gtokernliang sfc-gh-gtokernliang marked this pull request as ready for review December 12, 2024 22:51
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Dec 12, 2024
)

def _validate_attributes(final_attributes: dict[str, Any]):
_validate_selector_name(final_attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

also should validate that span types are a string too. Unless you plan on folding it into the TODO on L35.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I was thinking of folding it into the TODO - basically:

  1. If the span type is provided, it must match one of the provided span types in the semcov package.
  2. Otherwise it's unknown.

_validate_attributes(attributes_to_add)

for key, value in attributes_to_add.items():
span.set_attribute(key, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

this won't prepend any namespaces but maybe it's just okay to get the user to say the full attribute name (i.e. with the scopes) themselves. If that's your idea, I'm good with that (so long as later when we validate spans with a given span type follow the semantic convention of that span type and throw an error I'm good).

Copy link
Contributor Author

@sfc-gh-gtokernliang sfc-gh-gtokernliang Dec 21, 2024

Choose a reason for hiding this comment

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

updated to prepend namespace

Base automatically changed from garett/SNOW-1854060 to main December 21, 2024 19:03
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Dec 21, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Dec 21, 2024
with custom_app as recording:
test_app.respond_to_query("throw")

print(recording)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add assertions to verify the expected behavior instead of just printing recording. This will ensure the test checks if spans are correctly emitted and stored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants