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.

Sign up for GitHub

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

[BUG]: Added return of the original function result when tracer not defined #1268

Merged
merged 3 commits into from
Oct 24, 2023
Merged

[BUG]: Added return of the original function result when tracer not defined #1268

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a8ae67
fix: Added return of the original function result when no tracer is d…
tazarov Oct 22, 2023
e762178
fix: Made attributes compatible with OTEL attributes
tazarov Oct 23, 2023
75683c6
fix: Removed unused method and imports
tazarov Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions chromadb/telemetry/opentelemetry/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from functools import wraps
from enum import Enum
from typing import Any, Callable, Dict, Optional, Union
from typing import Any, Callable, Dict, Optional, Union, cast

from opentelemetry import trace
from opentelemetry.util import types
from opentelemetry.sdk.resources import SERVICE_NAME, Resource
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import (
Expand Down Expand Up @@ -61,6 +62,13 @@ def __init__(self, system: System):
granularity: OpenTelemetryGranularity = OpenTelemetryGranularity("none")


def _transform_attributes(
tazarov marked this conversation as resolved.
Show resolved Hide resolved
attributes: Dict[str, Any]
) -> Dict[str, types.AttributeValue]:
"""Transform attributes to the format expected by OpenTelemetry."""
return {k: cast(types.AttributeValue, v) for k, v in attributes.items()}


def otel_init(
otel_service_name: Optional[str],
otel_collection_endpoint: Optional[str],
Expand All @@ -72,8 +80,10 @@ def otel_init(
Parameters match the environment variables which configure OTel as documented
at https://docs.trychroma.com/observability.
- otel_service_name: The name of the service for OTel tagging and aggregation.
- otel_collection_endpoint: The endpoint to which OTel spans are sent (e.g. api.honeycomb.com).
- otel_collection_headers: The headers to send with OTel spans (e.g. {"x-honeycomb-team": "abc123"}).
- otel_collection_endpoint: The endpoint to which OTel spans are sent
(e.g. api.honeycomb.com).
- otel_collection_headers: The headers to send with OTel spans
(e.g. {"x-honeycomb-team": "abc123"}).
- otel_granularity: The granularity of the spans to emit.
"""
if otel_granularity == OpenTelemetryGranularity.NONE:
Expand Down Expand Up @@ -110,7 +120,7 @@ def wrapper(*args: Any, **kwargs: Dict[Any, Any]) -> Any:
if trace_granularity < granularity:
return f(*args, **kwargs)
if not tracer:
return
return f(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

with tracer.start_as_current_span(trace_name, attributes=attributes):
return f(*args, **kwargs)

Expand All @@ -129,4 +139,4 @@ def add_attributes_to_current_span(
if not tracer:
return
span = trace.get_current_span()
span.set_attributes(_transform_attributes(attributes)) # type: ignore
span.set_attributes(_transform_attributes(attributes))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this was sloppy on my part. The correct fix here is to simply remove the call to _transform_attributes.

The call to _transform_attributes is a holdover from when add_attributes_to_current_span accepted a Dict[str, Any], since OTel wants attribute values to be one of [str, bool, float, int]. @HammadB and I talked about this in code review and decided we should strongly type add_attributes_to_current_span so transformation is no longer necessary.

All that said: let's delete _transform_attributes and the call to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy is still complaining:

image

Should I do this instead?:

span.set_attributes({k: cast(types.AttributeValue, v)
                        for k, v in attributes.items()})

It seems that Otel expects AttributeValue so we might have to do the conversion.

Note: We can even widen the accepted values to include sequences of the ones you mentioned above:

AttributeValue = Union[
    str,
    bool,
    int,
    float,
    Sequence[str],
    Sequence[bool],
    Sequence[int],
    Sequence[float],
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is why I shouldn't review code on Sunday night ;)

I like your original method of solving this, or I'd be happy to cast to AttributeValue inline in add_attributes_to_current_span -- up to you. And sorry to have asked you to make an unnecessary change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the latest change, it works and feels more minimalistic. (we live and die with mypy)

Loading