-
Notifications
You must be signed in to change notification settings - Fork 272
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
Update function state change traces to improve orchestration monitoring query #2302
Conversation
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.
Thanks for doing this work! I added a request for a manual test, please see my comment below
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs
Outdated
Show resolved
Hide resolved
As an update, I removed the |
I looked into removing the additional state trace log and updating the existing I'm looking into updating the existing state change trace with a new |
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.
Left 2 small questions
src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj
Outdated
Show resolved
Hide resolved
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.
With the exception of the ResumingOrchestration
log, the code changes look good to me! It is certainly confusing that we log both a State
and RuntimeStatus
field. We'll need to tackle that eventually. Other than that, the overall change strategy makes sense to me. I think you should be good to update the test code
@davidmrdavid, I know the Netherite smoke test is expected to fail right now, but is the Python smoke test expected to fail? |
@bachuv: it isn't, but the error suggests the problem was a failure in downloading the docker image. I'm re-running the test now to see if it's transient. |
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.
LGTM so long as all smoke tests (except the Netherite one) pass
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.
LGTM!
…ng query (#2302) This PR adds a new RuntimeStatus field to track function state changes. This helps us improve our monitoring query in the portal (Functions -> OrchestratorFunction -> Monitor tab -> App Insights query).
This PR adds a new
RuntimeStatus
field to track function state changes. This helps us improve our monitoring query in the portal (Functions -> OrchestratorFunction -> Monitor tab -> App Insights query).Issues that are fixed in this PR:
Terminated
orchestrationsSuspended
runtime stateNew query to list the status of orchestration instances:
Pull request checklist
pending_docs.md
release_notes.md
/src/Worker.Extensions.DurableTask/AssemblyInfo.cs