8000 [3.x] feat: OhDear integration by kbond · Pull Request #289 · liip/LiipMonitorBundle · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[3.x] feat: OhDear integration #289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 15, 2024
Merged

[3.x] feat: OhDear integration #289

merged 1 commit into from
Feb 15, 2024

Conversation

kbond
Copy link
Collaborator
@kbond kbond commented Nov 24, 2023

No description provided.

@kbond
Copy link
Collaborator Author
kbond commented Nov 24, 2023

@Chris53897, as it looks like you'd use this, could you give a quick review?

@Chris53897
Copy link
Contributor

I will do as sonn if i have some free time. But we do not use LiipMonitorBundle in Production yet.

@kbond kbond force-pushed the feat/ohdear branch 2 times, most recently from 475d963 to 5f5b678 Compare November 28, 2023 19:12
@Chris53897
Copy link
Contributor

I just noticed that laminas/diagnostics is now compatible with PHP 8.3.
This makes testing for me easier, as we already moved all our projects and bundles to PHP 8.3 as minimum.
Will try to do it in the next days.

@Chris53897
Copy link
Contributor
Chris53897 commented Dec 24, 2023 •
edited
Loading

Thanks. It looks good.
Some ideas for improvements/considerations.

If i activate the check for system_memory_usage: true # warn @ 70%, fail @ 90% it results in crashed, because not supported on mac silicon accoring to the message.

The stackTrace is in the MetaData stack_trace.
Should this be excluded for security reasons?

The documentation is not clear if the correct status is error or failed.
See notificationMessage
https://ohdear.app/docs/features/application-health-monitoring#health-check-results-format
I did contact support of OhDear for this.

Maybe it is useful to make it easier in env=dev to show the health results as json. By direct call via Browser
Somethink like this, but needs a check for dev env. And on the downside, it logs the sectet in logs, and can leaked.
So i am not sure if there is a better solution
if ($this->secret !== $request->headers->get('oh-dear-health-check-secret') && $this->secret !== $request->get('oh-dear-health-check-secret')) {

I think a hint to add the route to the firewall is helpful.

checkResults is limited to 50 items. I am not sure how OhDear handle this, if there are more.
https://ohdear.app/docs/features/application-health-monitoring#health-check-results-format
I did contact support of OhDear for this.
Update: If there are more than 50, no import will be done.
Maybe sort the items by status? Failing and crashing first. Discard by 49 and optional add a warning that there are more than 50 checks?

@kbond
Copy link
Collaborator Author
kbond commented Jan 2, 2024 •
edited
Loading

Great, thanks for testing this!

If i activate the check for system_memory_usage: true # warn @ 70%, fail @ 90% it results in crashed, because not supported on mac silicon accoring to the message.

Do you think we should do anything here? I'm guessing it will work in production?

The stackTrace is in the MetaData stack_trace.
Should this be excluded for security reasons?

Good call - global config option perhaps? I guess in certain scenario's you may want the stack trace. Maybe an option when running the checks?

I think a hint to add the route to the firewall is helpful.

Can you describe this a bit more? The route needs to be public (or is that what you mean?).

Maybe sort the items by status? Failing and crashing first. Discard by 49 and optional add a warning that there are more than 50 checks?

Good idea, that seems like the best option.

@kbond
Copy link
Collaborator Author
kbond commented Jan 2, 2024

Maybe it is useful to make it easier in env=dev to show the health results as json.

What about wrapping the request header check into a protected method that you can override to add this type of logic?

@Chris53897
Copy link
Contributor

@kbond Sorry, for late response.
You have good Points.

Maybe we can merge this PR as it is, and after that tackle one task after the other?
This will make it easier to contribute for others.

Our symfony workers in live actual get stucked after some load, and we need to check it manual at the moment ;(
I would love to have a roundtrip of checking # workers running => LiipMonitoringBundle => OhDear => ReRun Workers on live, if stuck. Or a cronjob that checks # workers running and restart them if stuck.
But the other checks mentionted here, are useful as well.

zenstruck/messenger-monitor-bundle#11

@kbond
Copy link
Collaborator Author
kbond commented Feb 15, 2024

Maybe we can merge this PR as it is, and after that tackle one task after the other?

Good idea, merging.

@kbond kbond merged commit 3429f1c into liip:3.x Feb 15, 2024
@kbond kbond deleted the feat/ohdear branch February 15, 2024 22:16
This was referenced Feb 18, 2024
Merged
Merged
@kbond kbond restored the feat/ohdear branch February 18, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kbond @Chris53897
0