-
Notifications
You must be signed in to change notification settings - Fork 137
Several minor changes and OpenTelemetry support #77
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
Conversation
# Conflicts: # poetry.lock # pyproject.toml
| _default_build_id: Optional[str] = None | ||
|
|
||
|
|
||
| def load_default_build_id(*, memoize: bool = True) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was basically copied verbatim from workflow_service.py. Heavy scrutiny not needed (but welcome).
# Conflicts: # temporalio/client.py # temporalio/worker/workflow_instance.py # temporalio/workflow.py # temporalio/workflow_service.py
| @@ -0,0 +1,17 @@ | |||
| from .message_pb2 import ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in this temporalio/api directory can be ignored. It is auto-generated source.
|
After discussion, there are just too many problems with creating spans on replay. So here's the new rules and I think they are the best way to have a deterministic workflow tree and still handle everything cleanly. They are numbered for easy discussion. Rules
By following these rules, users should get all the benefits of having tracing without the annoyance of spans being created unnecessarily during replay. Sending to team for feedback. |
|
Marked ready for review. I have made updates to apply to the rules above. I will break this out into a bit clearer spec and we will make sure to document for newer SDKs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't go into much depth.
The spec and tests look good, so I'm approving.
What was changed
opentelemetryas an "extra" (i.e. optional set of dependencies)temporalio.contrib.opentelemetrypackage with an interceptor that supports OpenTelemetryworker_binary_idfrom being a client option tobuild_idbeing a worker optionidentityworker option to override one on the client if user wantsheaderparam for client calls (for interceptor use only currently)headerson interceptor inputsclient.describewasn't properly calledNoneor with no return weren't setting result payloadsworkflow.time()andworkflow.time_ns()calls to mimic the Pythontimelibrary equivalents. Could not just usedatetimeresult from existingworkflow.now()in Otel impl because I wanted nanosecond precision.eval_strininspect.signatureinstead oftyping.get_type_hintswhich has more support but is lower levelWhy?
Just about everything here was borne out of OpenTelemetry needs and things discovered during OpenTelemetry implementation. The implementation does a naive open-then-close span approach in non-replay code paths which causes problems in certain situations (see PR comments).
Checklist