-
Notifications
You must be signed in to change notification settings - Fork 104
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
opentelemetry
as an "extra" (i.e. optional set of dependencies)temporalio.contrib.opentelemetry
package with an interceptor that supports OpenTelemetryworker_binary_id
from being a client option tobuild_id
being a worker optionidentity
worker option to override one on the client if user wantsheader
param for client calls (for interceptor use only currently)headers
on interceptor inputsclient.describe
wasn't properly calledNone
or with no return weren't setting result payloadsworkflow.time()
andworkflow.time_ns()
calls to mimic the Pythontime
library equivalents. Could not just usedatetime
result from existingworkflow.now()
in Otel impl because I wanted nanosecond precision.eval_str
ininspect.signature
instead oftyping.get_type_hints
which 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