Datadog observability driver #998
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Overall looks pretty good! I love how little work was needed to extend @dylanholmes's existing observability work.
| ) | ||
| span_processor: SpanProcessor = field( | ||
| default=Factory( | ||
| lambda self: BatchSpanProcessor(OTLPSpanExporter(endpoint=self.datadog_agent_endpoint + "/v1/traces")), |
There was a problem hiding this comment.
Use f-string instead of string concatenation.
| takes_self=True, | ||
| ), | ||
| kw_only=True, | ||
| ) | ||
| _tracer: Optional[Tracer] = None | ||
| _root_span_context_manager: Any = None | ||
|
|
||
| def _trace_provider_factory(self) -> TracerProvider: | ||
| attributes = {"service.name": self.service_name} | ||
| if self.service_version: |
There was a problem hiding this comment.
Use explicit is not None check
| attributes = {"service.name": self.service_name} | ||
| if self.service_version: | ||
| attributes["service.version"] = self.service_version | ||
| if self.deployment_env: |
There was a problem hiding this comment.
Approved but would still like @dylanholmes's eyes before merging.
| @@ -18,18 +18,28 @@ | |||
|
|
|||
| @define | |||
| class OpenTelemetryObservabilityDriver(BaseObservabilityDriver): | |||
| service_name: str = field(kw_only=True) | |||
| service_name: str = field(default="griptape", kw_only=True) | |||
There was a problem hiding this comment.
Small refactoring opportunity: If we have a default here, then we don't really need it in GriptapeObservabilityDriver anymore (which also mean that we need to override trace_provider there too).
Though its fine by me if you don't want to do it in this PR
There was a problem hiding this comment.
I assumed GriptapeCloud used a non-attribute trace_provider on purpose. If we are OK with GriptapeCloud using the OpenTelemetry tracer_provider, then happy to. Just not clear why it used its own trace_provider in the first place
There was a problem hiding this comment.
tracer_provider was just added GriptapeObservabilityDriver in order to work around some attrs awkwardness. The resource arg was left out because Griptape Cloud doesn't really need them, but they don't hurt either. I'd personally prefer simpler code
Describe your changes
Add a Datadog observability driver that pushes observability traces to a Datadog agent.
Issue ticket number and link