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

Change event_payload column size from TEXT to MEDIUMTEXT #16751

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eduardoj
Copy link
Member

This change was missing when the payload colum size of table events was changed.

Related to #15649.

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Aug 26, 2024
This change was missing when the payload colum size of table events was
changed.
@eduardoj eduardoj force-pushed the fix/event_payload_text_to_mediumtext branch from e0749f5 to a0a624f Compare August 26, 2024 09:05
@eduardoj eduardoj marked this pull request as ready for review August 26, 2024 09:24
@darix
Copy link
Member

darix commented Aug 26, 2024

while I think this change is a good step in the right direction ... might i suggest a follow up PR which basically adds some exception handling so that it can skip over such events and try requests further down the list?

and maybe some influxdb metric that tells us about failed notifications.

@rubhanazeem rubhanazeem added the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Aug 26, 2024
@eduardoj
Copy link
Member Author

This was a first and fast approach to solve the issue of not creating notifications.

I put this PR in draft, and will think in a better solution.

@eduardoj eduardoj marked this pull request as draft August 26, 2024 10:25
@hennevogel
Copy link
Member

might i suggest ... some exception handling so that it can skip over such events and try requests further down the list?

Yeah delayed_job is not too smart about low level failures. I'm sure this will come up in the RCA :)

and maybe some influxdb metric that tells us about failed notifications.

We already measure everything about this, including alerts. They just fired when no one was there anymore...

https://obs-measure.opensuse.org/d/SKLo7evMk/emails?orgId=1
https://obs-measure.opensuse.org/d/N1ubohEMz/notifications?orgId=1
https://obs-measure.opensuse.org/d/zUE8_eM7k/activejob?orgId=1&refresh=5m

@darix
Copy link
Member

darix commented Aug 27, 2024

This was a first and fast approach to solve the issue of not creating notifications.

I put this PR in draft, and will think in a better solution.

TBH making all the tables use the same size for the columns is good. i was just thinking how we can handle this better if similar errors show up in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants