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

Improve progress reporting for download jobs #587

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Dec 20, 2024

Currently the reporting of progress for download jobs has some flaws:

  1. it concurrently updates a shared monitor but these are usually not made for concurrent use
  2. the jobs own monitor does not really reflect progress 3) Messages are not externalized

This now refactor this part in the following way:

  • create a submonitor from the job so we can update the remaning work
  • assign some subticks to each download to report progress on the job
  • set the current artifact behind downloaded as the message
  • report messages from downstream as sub task to the job
  • add two consumers for the caller of the job to get notified about messages and status

Copy link

github-actions bot commented Dec 20, 2024

Test Results

  250 files   -   125    250 suites   - 125   27m 33s ⏱️ - 21m 35s
1 904 tests ±    0  1 901 ✅ ±    0  3 💤 ±0  0 ❌ ±0 
4 473 runs   - 2 239  4 467 ✅  - 2 236  6 💤  - 3  0 ❌ ±0 

Results for commit b28f102. ± Comparison against base commit 93e609c.

♻️ This comment has been updated with latest results.

@laeubi laeubi force-pushed the improve_message_reporting_for_download_job branch from 5a88db0 to 8dbe338 Compare December 20, 2024 11:54
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

There's more logging, but there are mistakes, it's less readable with more redundancies:

Downloading Software
Downloading {0}com.google.guava.failureaccess 1.0.2
Downloading {0}org.apiguardian.api 1.1.2
Downloading {0}org.eclipse.platform_root 4.34.0.v20241120-1800
Downloading {0}javax.xml.jre 1.3.4.202210170008
Downloading javax.xml.jre
Downloading org.eclipse.platform_root
Downloading com.google.guava.failureaccess
Downloading org.apiguardian.api
Downloading org.eclipse.platform_root
Downloading com.google.guava.failureaccess
Downloading javax.xml.jre
Downloading org.apiguardian.api
Downloading com.google.guava.failureaccess - Fetching 202412041000&countryCode=us&timeZone=1&format=xml from https://www.eclipse.org/downloads/download.php?format=xml&file=/releases/2024-12/
Downloading org.eclipse.platform_root - Fetching org.eclipse.platform_root_4.34.0.v20241120-1800 from https://mirror.serverion.com/eclipse/releases/2024-12/202412041000/binary/ (196B)
Downloading {0}jakarta.inject.jakarta.inject-api 2.0.1
Downloading jakarta.inject.jakarta.inject-api
Downloading javax.xml.jre - Fetching javax.xml.jre_1.3.4.202210170008.jar from https://ftp.fau.de/eclipse/releases/2024-12/202412041000/plugins/ (9.18kB)
Downloading com.google.guava.failureaccess - Fetching com.google.guava.failureaccess_1.0.2.jar from https://mirror.dkm.cz/eclipse/releases/2024-12/202412041000/plugins/ (4.63kB)
Downloading {0}jakarta.inject.jakarta.inject-api 1.0.5
Downloading jakarta.inject.jakarta.inject-api
Downloading {0}org.eclipse.e4.ui.swt.win32 1.2.300.v20240416-0658

I think the new downloading message, the one with the mistake is simply not needed at all because that's already produced elsewhere.

For a given artifact I see quite a bit more stuff:

Downloading {0}com.google.guava.failureaccess 1.0.2
Downloading com.google.guava.failureaccess
Downloading com.google.guava.failureaccess
Downloading com.google.guava.failureaccess - Fetching 202412041000&countryCode=us&timeZone=1&format=xml from https://www.eclipse.org/downloads/download.php?format=xml&file=/releases/2024-12/
Downloading com.google.guava.failureaccess - Fetching com.google.guava.failureaccess_1.0.2.jar from https://mirror.dkm.cz/eclipse/releases/2024-12/202412041000/plugins/ (4.63kB)

Some of the details about the location aren't bad. But it's also a sea of long URLs now. The whole batch starts with

Collecting 586 artifacts from https://download.eclipse.org/releases/2024-12/202412041000

so arguably we don't need to see that URL 586 times in what follows.

I suppose I can try to filter/edit what's coming out. But the new message appears not to be needed at all and is broken.

@laeubi laeubi force-pushed the improve_message_reporting_for_download_job branch from 8dbe338 to c9c4ea2 Compare December 21, 2024 04:30
@laeubi
Copy link
Member Author

laeubi commented Dec 21, 2024

I suppose I can try to filter/edit what's coming out. But the new message appears not to be needed at all and is broken.

I think we have an inherit problem here that is how the message of a monitor is used/reported to the user:

  1. Your use-case seem to simply print out all messages in a linear stream. While it is of course not wrong, this is something very hard to get "right" as there is no context at all and it is very sensitive to any change.
  2. Then we have UI, where each job has its own visual presentation, and only one message is shown at a time so there is no "history" where we can look for additional information.

So currently I have crafted this with the second case in mind, so the user is getting more and more details depending how far the progress evolved.

So from your output a user will see:

  1. Downloading Software
  2. Downloading com.google.guava.failureaccess 1.0.2
  3. Downloading com.google.guava.failureaccess - Fetching 202412041000&countryCode=us&timeZone=1&format=xml from https://www.eclipse.org/downloads/download.php?format=xml&file=/releases/2024-12/
  4. Downloading com.google.guava.failureaccess - Fetching com.google.guava.failureaccess_1.0.2.jar from https://mirror.dkm.cz/eclipse/releases/2024-12/202412041000/plugins/ (4.63kB)

and then it will start with the next artifacts (at position 2) and so on but only ever one message at all.

Given that 1+2 are (hopefully) fast we can certainly drop this but there is a risk then that users are seeing outdated messages (and I'll try to filter out duplicated).

@laeubi
Copy link
Member Author

laeubi commented Dec 21, 2024

Downloading Software

What bugs me a bit is that you see that message at all it should not be produced by the "usual" monitor, do you hook into the Job framework maybe to get progress messages from there or do you call DownloadJobs directly?

Beside that I (hopefully) have restored maybe most previous behavior by do not forward the main task messages.

@laeubi laeubi force-pushed the improve_message_reporting_for_download_job branch 3 times, most recently from 481bc72 to a7738a7 Compare December 21, 2024 04:59
@merks merks self-requested a review December 21, 2024 06:49
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

For logging, Oomph already filters noisy messages.

https://github.com/eclipse-oomph/oomph/blob/d4d55f8570c0cc8f20b35da5866b8b56697db8c4/plugins/org.eclipse.oomph.setup/src/org/eclipse/oomph/setup/log/ProgressLogFilter.java#L19-L67

So now I can just filter the Downloading prefix and then in the log I see the following:

Fetching 202412041000&countryCode=us&timeZone=1&format=xml from https://www.eclipse.org/downloads/download.php?format=xml&file=/releases/2024-12/
Fetching org.eclipse.m2e.discovery_2.1.0.20241001-1350.jar from https://mirror.serverion.com/eclipse/releases/2024-12/202412041000/plugins/ (78.06kB)
Fetching org.eclipse.m2e.refactoring_2.1.0.20241001-1350.jar from https://ftp.halifax.rwth-aachen.de/eclipse/releases/2024-12/202412041000/plugins/ (78.68kB)
Fetching org.eclipse.mylyn.github.core_6.6.0.v20241002-1142.jar from https://mirrors.dotsrc.org/eclipse//releases/2024-12/202412041000/plugins/ (78.1kB)
Fetching org.eclipse.core.filesystem_1.11.100.v20241022-0806.jar from https://eclipse.mirror.garr.it/releases/2024-12/202412041000/plugins/ (77.16kB)
Fetching org.eclipse.equinox.p2.operations_2.7.400.v20240425-0751.jar from https://ftp.fau.de/eclipse/releases/2024-12/202412041000/plugins/ (80.15kB)
Fetching org.eclipse.mylyn.wikitext.mediawiki_4.5.0.v20241022-1702.jar from https://mirror.dkm.cz/eclipse/releases/2024-12/202412041000/plugins/ (80.34kB)
Fetching org.eclipse.equinox.p2.ui.sdk_1.3.300.v20240207-1113.jar from https://eclipse.mirror.liteserver.nl/releases/2024-12/202412041000/plugins/ (80.71kB)
Fetching org.eclipse.core.runtime_3.32.0.v20241003-0436.jar from https://mirror.ibcp.fr/pub/eclipse/releases/2024-12/202412041000/plugins/ (81.15kB)

It's actually rather interesting to see from which mirror each artifact is fetched; less interesting when there is no mirror involved.

Collecting 5 artifacts from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200
Fetching epp.package.java.executable.win32.win32.x86_64_4.34.0.20241128-0756 from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200/binary/ (276.79kB)
Fetching org.eclipse.epp.package.common.feature_4.34.0.20241128-0756.jar from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200/features/ (22.84kB)
Fetching org.eclipse.epp.package.java_4.34.0.20241128-0756.jar from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200/plugins/ (275.73kB)
Fetching org.eclipse.epp.package.common_4.34.0.20241128-0756.jar from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200/plugins/ (259.27kB)
Fetching org.eclipse.epp.package.java.feature_4.34.0.20241128-0756.jar from https://download.eclipse.org/technology/epp/packages/2024-12/202411281200/features/ (22.69kB)

But given that the artifact name is first, I think this is still really quite nice.

Also the progress monitor while resolving a target platform looks fine. It's really hard to screen capture the messages about fetching artifacts because they flash by really fast:

image

@laeubi laeubi force-pushed the improve_message_reporting_for_download_job branch from a7738a7 to a11a3ab Compare December 22, 2024 07:15
Currently the reporting of progress for download jobs has some flaws:

1) it concurrently updates a shared monitor but these are usually not
made for concurrent use
2) the jobs own monitor does not really reflect progress
3) Messages are not externalized

This now refactor this part in the following way:
- create a submonitor from the job so we can update the remaning work
- assign some subticks to each download to report progress on the job
- set the current artifact behind downloaded as the message
- report messages from downstream as sub task to the job
- add two consumers for the caller of the job to get notified about
messages and status
@laeubi laeubi force-pushed the improve_message_reporting_for_download_job branch from a11a3ab to b28f102 Compare December 26, 2024 15:17
@merks merks merged commit 70dc6a5 into eclipse-equinox:master Dec 27, 2024
12 checks passed
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.

3 participants