search

awslabs / amazon-kinesis-video-streams-pic

Apache License 2.0
49 stars 47 forks source link

State machine name for logs #233

Closed disa6302 closed 8 months ago

disa6302 commented 8 months ago

Issue #, if available:

Description of changes:

What was changed?

Why was it changed?

Our SDKs have multiple state machines running at once and it only logs the state in hex, which makes it confusing to understand which state machine transitions are being tracked while debugging. This PR introduces addition of tags when logging state machine transitions in VERBOSE mode to make it easy to understand which state machine transition the logs belong to.

How was it changed? Introduced a new API createStateMachineWithTag to pass in a tag. This is mandatory. if not provided the API call would fail.

Earlier:

2023-12-28 18:42:08.694 VERBOSE stepStateMachine(): State Machine - Current state: 0x0000000000000001, Next state: 0x0000000000000002, Current local state retry count [0], Max local state retry count [5], State transition wait time [0] ms

After this change:

2023-12-28 18:40:18.229 VERBOSE   stepStateMachine(): [STREAM] State Machine - Current state: 0x0000000000000010, Next state: 0x0000000000000040, Current local state retry count [0], Max local state retry count [5], State transition wait time [0] ms

Testing` Included API unit tests to confirm positive and negative test cases.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-commenter commented 8 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (411b9d8) 78.30% compared to head (083d394) 78.34%.

:exclamation: Current head 083d394 differs from pull request most recent head 6d972f2. Consider uploading reports for the commit 6d972f2 to get more accurate results

Files Patch % Lines
src/state/src/State.c 87.50% 2 Missing :warning:
src/client/src/Client.c 0.00% 1 Missing :warning:
src/client/src/Stream.c 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #233 +/- ## =========================================== + Coverage 78.30% 78.34% +0.03% =========================================== Files 52 52 Lines 10032 10065 +33 =========================================== + Hits 7856 7885 +29 - Misses 2176 2180 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hassanctech commented 8 months ago

This is super useful, thanks for doing this @disa6302 ! I have a concern though about truncation if the user specifies a tag longer than the max allowed length. It feels wrong to return a status success when we failed to use the tag specified by the user. I think in this case we should return a failure status and not set a tag at all. If no tag is set we should not have the extra []. It would be nice to make the tag required but would break backwards compat...

disa6302 commented 8 months ago

This is super useful, thanks for doing this @disa6302 ! I have a concern though about truncation if the user specifies a tag longer than the max allowed length. It feels wrong to return a status success when we failed to use the tag specified by the user. I think in this case we should return a failure status and not set a tag at all. If no tag is set we should not have the extra []. It would be nice to make the tag required but would break backwards compat...

I thought about this. I really wanted to add it as part of createStateMachine and not have a separate API. I am ok with adding the return code to fail on this, but given its a tag to uniquely identify a state machine log line, I thought it is ok to truncate (something is better than nothing ideology) instead of setting the tag again in case of failure.