-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Observability LGTM dev service filling up logs with services starting #45690
Observability LGTM dev service filling up logs with services starting #45690
Conversation
de89284
to
6957299
Compare
@edeandrea nice catch and fix. Lgtm! |
I was looking for a way to tie the container logs into the
|
Yeah, I was playing with that idea once ... but ended up with the same problem as you. Or in my case it was at first that there was no useful info / log, and then I managed to fix this. |
Nah, I think your stuff is the way to go. There should be more then just LGTM stuff in Observability Dev Services / Resources -- I need to find the time to merge back 2nd (and probably 3rd) part of my initial (huge) Observability Dev Services PR ... |
That was what I was thinking too, so that's the route I took with introducing the |
Status for workflow
|
Could it be something with #45689 ? |
I honestly don't know if it's related. It doesn't seem to be, but like @holly-cummins mentioned I don't know this codebase very well. This is the first time I've dug into it at all. But that PR hasn't yet been merged? Or are you saying there's something pre-existing that PR is intending to fix. Like I said on my machine I ran all those interaction tests successfully. I ran the entire project, not just that one test. |
No.
Yes, or at least, that's how I understood @holly-cummins comment(s). |
Either way though wouldn't that mean that nothing I did in this PR is triggering this failure? |
Yeah, I don't see how your changes could produce this error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build error is due to a timeout in tests.
See: https://github.com/quarkusio/quarkus/actions/runs/12837447370/job/35801488131?pr=45690#step:17:49213
It's likely a transient.
It sounds like @brunobat has diagnosed the failure in this build. The problem my change was intending to fix is that if the lgtm container isn't shut down properly (either because of a test problem, or because a person left it up deliberately), that container will be re-used as a dev service, but clients won't be able to connect to all services. (They'll try use default ports for some services instead of container-specific ones from config.) |
The re-use will need to provide the ports to use somehow... Otherwise the services connection will fail, @holly-cummins |
That's what my changes on #45689 do. Well, sort of. Most of it was already there; @alesj already had code which worked backwards from the ports exposed on the container to the correct config for that container, but it missed setting |
Excelent! @holly-cummins |
Sorry but could you clarify why this ends up behaving differently than all the other Dev Services? |
@gsmet which aspect of the behaviour do you mean (are you asking about the logging, or the "work backwards to allow re-use" model, or the "fails to shut down with Holly's classloading changes" part?) |
At startup the Observability LGTM devservice was filling the console with lots of logs:
Furthermore, the wait strategy was only waiting for the grafana http port to be available, which is available very soon in the container startup lifecycle. Instead, it should be waiting for all the services in the container to start up.
After this change the console now looks like this:
And the
Lgtm Dev Services Starting:
message stays at the bottom of the console until its fully up and running.