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

Add reason to GET /status response #86

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Add reason to GET /status response #86

merged 2 commits into from
Jul 25, 2023

Conversation

FragLegs
Copy link
Contributor

As discussed in our most recent call, this PR adds an OPTIONAL "reason" to the response from a GET /status request.

The other work of removing subjects from the /status endpoint will come in a later PR because I suspect it may be more contentious.


> A string whose value MUST be one of the values described below.

reason
Copy link
Contributor

@appsdesh appsdesh Jul 21, 2023

Choose a reason for hiding this comment

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

Thinking out loud here. Should we support I8N style reason similar to CAEP and RISC events?

            "reason": {
                "en": "System is down"
            },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting idea. I was imagining that this reason would usually come from the Receiver via the reason field in the POST /status endpoint, but might sometimes come from an internal process within the Transmitter. Would we need to push that i8n to the POST /status endpoint as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the reason is for the human consumption it would make sense to include it in the POST request from the receiver during stream update call as well.

If this API is widely adopted and we are concerned about the compatibility issue while updating the request input then we could use a different key like receiver_ingested_reason or something similar in the POST request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some more time thinking about this over the weekend. I think that we should assume that "reason" acts like an enum. That is, whether a Receiver or Transmitter changes the status of the stream, it will be because of a programmatic response to events with a small set of possible reasons. We should assume that Transmitters will make known the possible reasons and Receivers will be aware of their own possible reasons. These strings are not really for human consumption, so I don't think we need to internationalize them.

Does that make sense to you? I can update the example reason to something like SYSTEM_DOWN to make it more obvious how we intend for these to be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me! Thanks for spending cycles. LGTM

@FragLegs FragLegs merged commit 03e926c into main Jul 25, 2023
2 checks passed
@FragLegs FragLegs deleted the add-reason branch July 25, 2023 16:56
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.

4 participants