Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

animals as text instead of emoji #4184

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jayunit100
Copy link
Contributor

@jayunit100 jayunit100 commented Dec 15, 2022

fixes #4183
old output:

15T02:32:53Z [ℹ]  🦊 - building plugin at path "cmd/cli/plugin/package"
15T02:32:53Z [ℹ]  �$ /usr/local/go/bin/go mod download
...

new output:

2022-12-15T14:42:11-05:00 [ℹ]  chicken - building plugin at path "cmd/cli/plugin-admin/test"
2022-12-15T14:42:11-05:00 [ℹ]  sheep   - building plugin at path "cmd/cli/plugin-admin/builder"
2022-12-15T14:42:11-05:00 [ℹ]  pony    - building plugin at path "cmd/cli/plugin-admin/codegen"
2022-12-15T14:42:11-05:00 [ℹ]  chicken$ /usr/local/go/bin/go mod download
2022-12-15T14:42:11-05:00 [ℹ]  pony   $ /usr/local/go/bin/go mod download
2022-12-15T14:42:11-05:00 [ℹ]  sheep  $ /usr/local/go/bin/go mod download

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #4184 (ac28cc4) into main (6cdbd30) will decrease coverage by 0.90%.
The diff coverage is n/a.

❗ Current head ac28cc4 differs from pull request most recent head fb1fb1b. Consider uploading reports for the commit fb1fb1b to get more accurate results

@@            Coverage Diff             @@
##             main    #4184      +/-   ##
==========================================
- Coverage   49.55%   48.65%   -0.90%     
==========================================
  Files         452      482      +30     
  Lines       44849    46969    +2120     
==========================================
+ Hits        22225    22853     +628     
- Misses      20513    21954    +1441     
- Partials     2111     2162      +51     
Impacted Files Coverage Δ
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <0.00%> (ø)
cmd/cli/plugin/isolated-cluster/main.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/get_node_pools.go 10.52% <0.00%> (ø)
.../isolated-cluster/imgpkginterface/client_imgpkg.go 0.00% <0.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
...lugin/isolated-cluster/fakes/client_imgpkg_fake.go 32.75% <0.00%> (ø)
cmd/cli/plugin/cluster/main.go 0.00% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
cmd/cli/plugin/cluster/machinehealthcheck.go 100.00% <0.00%> (ø)
cmd/cli/plugin/isolated-cluster/test/main.go 0.00% <0.00%> (ø)
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jmoroski
Copy link
Contributor

If these are truly just concurrency identifiers, can't we just use "Thread 01-Thread 12", or "Job 01-Job 12" (or with zero-based numbering if you prefer)?

@jayunit100
Copy link
Contributor Author

jayunit100 commented Dec 15, 2022

i like the animal names but if its a "hard no" then ill replace w/ ints let me know @jmoroski .

In all seriousness, here is my case for keeping the "animal spirit" alive:

  • they are easier to read /search / reference then arbitrary integers.
  • the intent of the emoji's in the original framework, to have a little creative expression in an otherwise mundane build system, are IMO worth preserving.

either way is fine tho :)

just lmk if you'd strongly prefer int IDs, ill do:

T1
T2
T3
T4
...

so theyr unique and searchable...

@jmoroski
Copy link
Contributor

... but if its a "hard no" then ill replace w/ ints

If this was internal only, I'd tell you to use whatever tasteful collection of animals you choose.

But I believe this builder plugin is what we currently tell people outside of our team (and possibly organization) to use to scaffold and build plugins. So IDs is the less fun, but better choice. I'm fine with your T1, T2, ...Tn option.

@jayunit100
Copy link
Contributor Author

Ack yup t1 is traceable no problem will fix it to that

@jayunit100
Copy link
Contributor Author

fixed the emjois to T1-9 !

@knabben
Copy link
Member

knabben commented Dec 21, 2022

LGTM

Copy link
Contributor

@jmoroski jmoroski left a comment

Choose a reason for hiding this comment

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

One minor nit, if you do 0 padding (T01, T02...T12) there will be a known fixed width for log parsers.

Take it or leave it, this is approved.

@marckhouzam
Copy link
Contributor

Thanks @jayunit100 . Looking specifically at the output I think we have some inconsistencies. This is what I see:

2022-12-21T15:52:51-05:00 [ℹ]  building local repository at artifacts/darwin/arm64/cli
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 0 for &{cmd/cli/plugin cluster 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 1 for &{cmd/cli/plugin feature 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 2 for &{cmd/cli/plugin isolated-cluster 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 3 for &{cmd/cli/plugin login 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 4 for &{cmd/cli/plugin managementcluster 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 5 for &{cmd/cli/plugin package 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 6 for &{cmd/cli/plugin pinniped-auth 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 7 for &{cmd/cli/plugin secret 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  	starting compile thread 8 for &{cmd/cli/plugin telemetry 2147483648 <nil>}
2022-12-21T15:52:51-05:00 [ℹ]  T6 - building plugin at path "cmd/cli/plugin/login"
2022-12-21T15:52:51-05:00 [ℹ]  T3 - building plugin at path "cmd/cli/plugin/cluster"
2022-12-21T15:52:51-05:00 [ℹ]  T8 - building plugin at path "cmd/cli/plugin/package"
2022-12-21T15:52:51-05:00 [ℹ]  T10 - building plugin at path "cmd/cli/plugin/secret"
2022-12-21T15:52:51-05:00 [ℹ]  T7 - building plugin at path "cmd/cli/plugin/managementcluster"
2022-12-21T15:52:51-05:00 [ℹ]  T9 - building plugin at path "cmd/cli/plugin/pinniped-auth"
2022-12-21T15:52:51-05:00 [ℹ]  T8$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T6$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T5 - building plugin at path "cmd/cli/plugin/isolated-cluster"
2022-12-21T15:52:51-05:00 [ℹ]  T9$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T5$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T4 - building plugin at path "cmd/cli/plugin/feature"
2022-12-21T15:52:51-05:00 [ℹ]  T7$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T10$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T4$ /Users/kmarc/.asdf/shims/go mod download
2022-12-21T15:52:51-05:00 [ℹ]  T3$ /Users/kmarc/.asdf/shims/go mod download

I realize this is nit-picky, but since we have our hands in there, might as well...

  1. Since T1 seems to mean thread 1 and right above we have a (new from last week) printout saying starting compile thread X, I find it a bit confusing the the two number don't match for the plugin being compiled.
  2. I also find that the output that includes $ looks surprising now that it is not an emoji preceding it. For example T8$ make it look like the $ is part of the ID. How about replacing the $ with - like for the other lines?
  3. I like @jmoroski suggestion of padding with a 0 to get some nicer alignment in the above.
  4. I believe the [i] which follows the timestamp will also not show properly for dumb terminals. Not much we can do about that at the moment though as it comes from the logging library.
  5. Not related to your change, but this new printout starting compile thread 1 for &{cmd/cli/plugin feature 2147483648 <nil>} does not have a very nice second part, does it? Should we fix it?

Just some suggestions...

@jayunit100
Copy link
Contributor Author

Now im starting to think Maybe I should role this in w a broader klog replacement :)

@vuil
Copy link
Contributor

vuil commented Dec 22, 2022

I echo @marckhouzam 's thoughts. The padding for better alignment, using matching ids for the "starting.." log line, as well as tweaking the $ looks straightforward enough to add.

@jayunit100
Copy link
Contributor Author

just saw this, fixed mark's suggestions... but im also going to look into klog more broadly as well today.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get rid of "github.com/aunum/log" emoji animnal face logs !
6 participants