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

[WIP] OpenTelemetry First Pass #242

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from
Draft

Conversation

mrkaye97
Copy link

@mrkaye97 mrkaye97 commented Nov 2, 2024

Better to review without whitespace

Important: There are likely some breaking changes in here (especially with Pydantic), so I'd need to do a fair amount more validation before being willing to merge

Comment on lines 151 to 158
try:
otel_exporter_oltp_headers = json.loads(_oltp_headers) if isinstance(_oltp_headers, str) else _oltp_headers
except json.JSONDecodeError:
otel_exporter_oltp_headers = None
Copy link
Author

Choose a reason for hiding this comment

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

this is a hack - I'm sure there's a nicer way to do this (basically need to get the headers in some structured format and parse them to a dict)

from typing import Any


def flatten(xs: dict[str, Any], parent_key: str, separator: str) -> dict[str, Any]:
Copy link
Author

Choose a reason for hiding this comment

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

Python annoyance: In general I try to avoid using Any, but type hinting for arbitrarily nested dictionaries is really clunky AFAIK

action_func = self.action_registry.get(action_name)
async def handle_start_step_run(self, action: Action):
with self.otel_tracer.start_as_current_span(
"hatchet.worker.runner.runner.Runner.handle_start_step_run"
Copy link
Author

Choose a reason for hiding this comment

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

would love input on naming 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.

1 participant