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

feat: add reasons to AirbyteStreamStatusTraceMessage #74

Merged
merged 7 commits into from
May 16, 2024

Conversation

subodh1810
Copy link
Contributor

@subodh1810 subodh1810 commented May 3, 2024

Tech spec
Protocol review doc

Sample JSON

{
    "type": "TRACE",
    "trace": {
        "type": "STREAM_STATUS",
        "emitted_at": 1.714638052064E12,
        "stream_status": {
            "stream_descriptor": {
                "name": "stream-name",
                "namespace": "stream-namespace"
            },
            "status": "INCOMPLETE",
            "reasons": [{
                "type": "RATE_LIMITED",
                "rate_limited": {
                    "quota_reset": 1714638052064
                }
            }]
        }
    }
}

@subodh1810 subodh1810 self-assigned this May 3, 2024
@subodh1810 subodh1810 marked this pull request as ready for review May 3, 2024 12:05
@subodh1810 subodh1810 changed the title feat: add RATE_LIMITED enum + metadata to AirbyteStreamStatusTraceMessage feat: add reasons to AirbyteStreamStatusTraceMessage May 8, 2024
Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

Ship it!

properties:
quota_reset:
description: "Optional time in ms representing when the API quota is going to be reset"
type: number
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason this isn't just a big int? do we need decimals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens I am just following the convention. We are using number for timestamp attributes. For instance take a look at the emitted_at in AirbyteTraceMessage, its defined as a number

      emitted_at:
        description: "the time in ms that the message was emitted"
        type: number

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgardens @subodh1810 This looks like a bug in the AirbyteTraceMessage. It was not obvious to me that integer was not an integer data type and is actually a big integer or that number is a floating point/double. Should we attempt to fix the AirbyteTraceMessage either as part of this PR or in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • let's do it correctly here and use integer.
  • let's create an issue to fix airbyte trace meesage and add a todo in the code that that should have been number. don't think this is a good moment to try to fix this old mistake. just want to do it right going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens @jdpgrailsdev added the TODOs in the protocol yaml and created a github issue
https://github.com/airbytehq/airbyte/issues/38163

@subodh1810 subodh1810 requested a review from cgardens May 12, 2024 11:58
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@subodh1810 subodh1810 merged commit fe0ada2 into main May 16, 2024
6 checks passed
@subodh1810 subodh1810 deleted the add-rate-limited-enum-trace-message branch May 16, 2024 14:04
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