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

Clear names for receivers API fields #12

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

santihernandezc
Copy link

We're using lastNotify and lastNotifyDuration to indicate the time and duration for the last attempt regardless of the outcome. A better option would be to use lastAttempt and lastAttemptDuration instead.

The field lastError doesn't store the last error that occurred, but the error for the last attempt to notify (if any). This could be confusing because if a notification fails to deliver once but succeeds afterwards, this field will be empty. The name has been changed to lastAttemptError to better reflect this behavior.

api/v2/openapi.yaml Outdated Show resolved Hide resolved
type: string
format: date-time
lastNotifyDuration:
lastAttemptDuration:
Copy link

Choose a reason for hiding this comment

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

Suggested change
lastAttemptDuration:
lastAttemptDuration:
description: duration of the last attempt to use the integration in humanized format, e.g. `1s` or `15ms`

Copy link
Author

Choose a reason for hiding this comment

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

@yuri-tceretian I just added descriptions for the fields and generated the files.

api/v2/openapi.yaml Outdated Show resolved Hide resolved
@gotjosh
Copy link
Collaborator

gotjosh commented Sep 30, 2022

While we don't necessarily need to stick 1:1 with Prometheus, it'd be good if similar things keep similar names. In prometheus, lastError https://github.com/prometheus/prometheus/blob/main/web/api/v1/api.go#L815-L818 is used for targets and conveys the same meaning as "last attempt".

Personally, I'd love to stick with lastError et al and a more clear description of the field that anyone can see in the API Spec but can see the argument for a more verbose approach. I'll leave it up to you ya'll, but wanted to provide some context.

api/v2/api.go Outdated Show resolved Hide resolved
Copy link

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM

@santihernandezc santihernandezc merged commit d75bdc5 into main Sep 30, 2022
@santihernandezc santihernandezc deleted the clear_naming_receivers_api branch September 30, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants