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

MNG-7875 colorize transfer messages #1231

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Conversation

hboutemy
Copy link
Member

@hboutemy hboutemy commented Sep 10, 2023

https://issues.apache.org/jira/browse/MNG-7875
before:
MNG-7875_before_output

after:
MNG-7875_colorized_output_second

(initial proposal:
MNG-7875_colorized_output
)

@hboutemy hboutemy force-pushed the MNG-7875_transfer_color branch 2 times, most recently from e90af14 to a6de9b0 Compare September 10, 2023 16:27
@rmannibucau
Copy link
Contributor

Not sure it is easier to read, what about keeping them at debug/fine level and use something maven oriented by default (info: Downloading org.apache.maven:foo:1.2.3 from apache-snapshots, here colors can optionally be used on the gav and repoId if needed but it is already readable without anything else IMHO)?

The fact it does not use a logger already messes up the output depending the build mode and mojos (due to stdout usage directly) so maybe migrate to slf4j impl first then use the levels and be it, looks better to solve the issue than working it around with colors which don't really enhance it for end users in the previous mentionned case(s).

@hboutemy
Copy link
Member Author

hboutemy commented Sep 10, 2023

using slf4j vs console is completely orthogonal: it does not change anything to this issue. Don't hesitate to open a Jira and a PR in parallel

on Downloading org.apache.maven:foo:1.2.3 from apache-snapshots:

  • we don't have gavce in the API: we can't do that
  • effective url base is useful (depends on user settings)
  • gavce can't display maven-metadata.xml

it is already readable without anything else IMHO

for sure, it's done as pure color enhancement, absolutely not any other change: then data exactly is the same, just color to help see the important different fields and cases

ask a normal user about the difference between "Downloading" and "Downloaded", ask about the url structure
for you and every Maven developer, there is no problem (just hard to read output: you need to concentrate to see the different parts)

I know this output is new and made by someone else: take time to use it and see live why it was done the way of was done...

@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {

@Override
public void transferInitiated(TransferEvent event) {
String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Copy link
Member

@cstamas cstamas Sep 10, 2023

Choose a reason for hiding this comment

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

If we want to cut logging (and I do agree with @elharo we log way too much), I'd just remove this method (make it no op).

Or just leave it for PUT (leave it as before), and do not log for the rest.

Reason is simple: if you have 5 reposes, you will have 5 "Downloading...." but only one "Downloaded", and the latter matter.s

Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate discussion. Foe this we have other listeners.

Copy link
Member Author

Choose a reason for hiding this comment

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

this line is not about cutting logging: it's about adding color when color enabled

Copy link
Member

@cstamas cstamas Sep 10, 2023

Choose a reason for hiding this comment

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

Well JIRA currently states following:

transfer message are currently hard to read for many users
(logging example cut for brevity)

  • it's interweaved into normal build messages
  • users don't really see the difference between "Downloading" (transfert started, may eventually fail with 404) and "Downloaded" (done successfully)
  • repository id is not so visible in the middle of the message
  • the download url has much info in it to see: base url, groupId as directory, artifactId, version, and filename

So, based on this above, I assumed, that: is interweave and too chatty, so cut it down, and if "users don't see the difference", they are right: As I said, this logger when 5 repositories are defined will emit 5 "Downloading" and 1 "Downloaded", and the one "Downloaded" matters, the 5 "Downloading" does not (better check with effective-pom what reposes you have instead to repeat it to each artifact you need to download).

Unsure how JIRA comes to conclusion based on these 4 bullets above "ok, let's color it!", when it is ONLY solving the last bullet (is not solving it actually, as when this listener is involved, layout is already applied, and we do not speak about "artifacts" but about "resources" that are resolved against some baseUrl/repository, so this is another problem).

Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate discussion. Foe this we have other listeners.

What other listeners we have for this? WDYM?

Copy link
Member

Choose a reason for hiding this comment

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

As we know, by default Maven "leaks" your artifact paths, and only reason "Downloading" log line is useful is only this one: it makes user aware Maven is making HTTP request against the logged baseUrl. OTOH, due overwhelming amount of Maven logging, this information is usually lost, due log-noise.

To properly fix "leaking", use RRF but that is another thing.

@rmannibucau
Copy link
Contributor

using slf4j vs console is completely orthogonal: it does not change anything to this issue. Don't hesitate to open a Jira and a PR in parallel

Yes and no, using colors does not solve the issue you mention for me, using loggers is a proposal so somehow it does IMHO.

we don't have gavce in the API: we can't do that

We can for 2 reasons:

  • We own aether
  • Worse case a thread local solves that since transport is sadly for now

effective url base is useful (depends on user settings)

Agree, this is why I said it would be debug level cause most of the time it is not but if needed it can be enabled with --loggin-level which sounds like solving the issue you reference for me.

gavce can't display maven-metadata.xml

Why? you can reference it in the mesage but it stays readable compared to an url randomized on the output.

it is already readable without anything else IMHO

Ok, I disagree on that point for the two mentionned reasons

for sure, it's done as pure color enhancement, absolutely not any other change: then data exactly is the same, just color to help see the important different fields and cases

This is why for me it is not better, several cases will not have colors, contrast will not always make it better - can make it worse btw - and length stays the same.

ask a normal user about the difference between "Downloading" and "Downloaded", ask about the url structure
for you and every Maven developer, there is no problem (just hard to read output: you need to concentrate to see the different parts)

Feedback I encountered were, rest was either "it is too verbose but I filtered it after or it was ok":

  • It is not readable - note: literally - cause of the output interleaving I mentionned
  • what is the link with my pom (repo in parents)

I know this output is new and made by someone else: take time to use it and see live why it was done the way of was done...

I appreciate the proposal but my point is I think it does not address the referenced ticket at the end - and I'm saying that after being one to think about that some years ago - was with sed to add colors at that time. Just trying to ensure we dont mark resolved a ticket we didnt address.

@hboutemy
Copy link
Member Author

disagree on everything: the PR does what the Jira title and description say = color to ease understanding
which solve my improvement expectation

your expectations are different, probably complementary, then to be done separately if you need it: going to slf4j will have other independent impact
I'm not saying that your expectations are wrong: just saying that they are independent and should not be conflated (or even imposed on something independent)

@rmannibucau
Copy link
Contributor

@hboutemy not imposing anything, was just sharing my feedback that I think this PR is a noop regarding the goal which is something I share. Due to the regular feedback we have on this part I don't think this step is that useful and I think we can skip it but no issue for me if you want to merge it - code is more than ok. It is just it does not reach its real goal and just push back the actual issue IMHO so thought it would be worth thinking to a global solution instead of a quick workaround.

@hboutemy
Copy link
Member Author

hboutemy commented Sep 11, 2023

the goal of this PR is https://issues.apache.org/jira/browse/MNG-7875 (please take time to read it), then this PR reaches its goal

if there is another goal (and I understand there is), please open a Jira issue describing the goal: I don't get fully this other goal. And of course, this PR does not try to address this other goal

@laeubi
Copy link

laeubi commented Sep 11, 2023

If the problem is "transfer message are currently hard to read for many users" I must confess that I agree that adding color make them not easier to read (for me as a user).... and maybe one should better try to enhance the message?

@gnodet
Copy link
Contributor

gnodet commented Sep 11, 2023

Another possibility would be to implement something different when in interactive mode:

  • when some downloads starts, print a line saying saying Maven is downloading
  • eventually put some progression, but all on a single line (i.e. only the last line is ever modified
  • when downloads are done, completely erase the download line and continue with normal build log
    This can be easily achieved using JLine... (as has already been proven to work inside Maven Daemon)

As a user, I'm usually not interested in knowing which exacts artifacts are downloaded, but mainly by the fact that the build is actually downloading artifacts.

@cstamas
Copy link
Member

cstamas commented Sep 11, 2023

As a user, I'm usually not interested in knowing which exacts artifacts are downloaded, but mainly by the fact that the build is actually downloading artifacts.

...completely agree, except with this last line (where am more at @hboutemy side): baseUrl used in download is good to be visible, as a cautious developer can catch from very first few lines of downloads if the env is not right (ie. mirror not properly set up, etc). This is why I keep saying that am personally uninterested in the "Downloading..." line, but am very interested in "Downloaded..." line.

Thinking more, I kinda agree with @gnodet to keep build logs unspoiled with downloading/downloaded lines (just to have some deleted line, so progress should be visible), and maybe at the build end (a la validation) have some summary table? Like by repoId/baseUrl/countOfDowmloads?

Basically, current Maven logging could be called "detailed" (as Maven 3.9.x behaves today), and introduce something like maven.download.logging=summary where NO downloading/downloaded are emitted (just progress that is deleted), but only a summary at build end?

@michael-o
Copy link
Member

Please note that we do have different listeners already which are activated through other flags indirectly.

@cstamas
Copy link
Member

cstamas commented Sep 11, 2023

Please note that we do have different listeners already which are activated through other flags indirectly.

Can you elaborate? I already asked you about it: #1231 (comment)

@cstamas
Copy link
Member

cstamas commented Sep 11, 2023

Also, I am willing to accept following PR:

  • for 3.9: emphasize (with color) the repoId and baseUrl, leave everything else as is and make no assumptions about layout
  • for 4: do something along the lines of @gnodet idea, combined with "summary table" maybe (discuss it on ML)

@michael-o
Copy link
Member

Please note that we do have different listeners already which are activated through other flags indirectly.

Can you elaborate? I already asked you about it: #1231 (comment)

Look at impl if the interface. We have for nop, slf4j, no progress and this one for the tty.

@hboutemy
Copy link
Member Author

dropped the layout-specific colorization as asked

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Please provide updated screenshots.

@hboutemy
Copy link
Member Author

here it is:
MNG-7875_colorized_output_second

probably less surprising to end-users

@hboutemy hboutemy merged commit 0c8b7df into maven-3.9.x Sep 11, 2023
29 checks passed
@hboutemy hboutemy deleted the MNG-7875_transfer_color branch September 11, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants