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

Add RFC to unify log output #308

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

Conversation

c0d1ngm0nk3y
Copy link
Contributor

@c0d1ngm0nk3y c0d1ngm0nk3y commented Aug 16, 2024

Summary

There are 2 different ways logging is done in the buildpacks:

  • buildpacks based on packit/v2 seem to use packit/v2/scribe
  • buildpacks based on libpak seem to use libpak/bard

Use Cases

Using a meta buildpack containing buildpacks from both approaches, e.g see npm sample

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@dmikusa
Copy link
Contributor

dmikusa commented Aug 18, 2024

I agree that it's not ideal to have two different output formats. It looks disjointed and makes it harder to follow what is happening.

The hard problem is what to do about it. I wasn't around for the original discussions around the output format so I don't have context on why it's like this. I'm sure there are reasons. If anyone knows and can elaborate on them, I think that would be helpful, so we can figure out a plan that tries to address all of the concerns.

From visual inspection, the most notable difference is color vs no color. What other differences are there? @c0d1ngm0nk3y it might help to include sample outputs of each in the RFC, and a suggested new format as well, so we can see the present state and what we're working towards.

@c0d1ngm0nk3y
Copy link
Contributor Author

I agree that it's not ideal to have two different output formats. It looks disjointed and makes it harder to follow what is happening.

The main problem for me is that the logger from libpak makes it very easy to see to which buildpack the log line belongs. This is harder for the packit logger, but a combination of both is worse. It looks like the logging is from one buildpack, but it might be from 2.

The hard problem is what to do about it.

Is it? The hard part might coming to a decision, but the actual doing should be rather easy. Correct me if I am missing something. I see 3 options:

  1. (preferable) Having a common library. Maybe not logging specific to add more common stuff in there over time. I think we could just start with a copy of libpak/bard. Migrating from one log library to another shouldn't a big deal.

  2. Every buildpack uses libpak/bard. So many buildpacks would already be fine. Using packit and libpak/bard should be posible, right?

  3. Align the log output to look identical. This would probably be the most effort and least benefit.

I wasn't around for the original discussions around the output format so I don't have context on why it's like this.

I'm sure there are reasons. If anyone knows and can elaborate on them, I think that would be helpful, so we can figure out a plan that tries to address all of the concerns.

tbh, I am not so sure that there are reasons and I don't call personal preferences a valid reason.

From visual inspection, the most notable difference is color vs no color. What other differences are there?

I guess that is the main point. Together with the styling (e.g. italics), it makes the log easier to read.

@c0d1ngm0nk3y it might help to include sample outputs of each in the RFC, and a suggested new format as well, so we can see the present state and what we're working towards.

Good point. I have added a screenshot.

Copy link

@loewenstein loewenstein left a comment

Choose a reason for hiding this comment

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

I like the proposal and agree that the current state is bad.

We should probably use the RFC phase to agree on a concrete implementation, shouldn't we?

Hence this is a LGTM without setting the story approved.

@dmikusa
Copy link
Contributor

dmikusa commented Sep 13, 2024

Is it? The hard part might coming to a decision,

100%, this is the hard part.

(preferable) Having a common library. Maybe not logging specific to add more common stuff in there over time. I think we could just start with a copy of libpak/bard. Migrating from one log library to another shouldn't a big deal.

I agree this would be ideal, but I don't see this happening until we solve the problem of having libpak & packit.

Every buildpack uses libpak/bard. So many buildpacks would already be fine. Using packit and libpak/bard should be posible, right?

Personally, this would be the easiest :) since all the buildpacks I help maintain already use this, but it's unfair for those working on buildpacks that don't. I don't think it's reasonable to expect them to just switch (if this were commercial software we could do that, but not with OSS software).

I also don't know about having libpak/packit take dependencies on each other. Maybe that would work, but it makes me concerned that you get into a weird situation where buildpacks are not consistently using one implementation (libpak or packit), and it makes them harder to understand/work with. It also makes me concerned about the possibility of circular dependencies. I'd rather see them stay totally separate.

Align the log output to look identical. This would probably be the most effort and least benefit.

It's a little more work up front to match the outputs, but logging never really changes so it's a one-time effort. If we just change the logging implementation to output logging in the same way as libpak, then no buildpacks have to change. Next time packit is updated, the format changes and everything looks consistent.

This would be my vote, but I feel like that means very little since it doesn't impact me.

We 100% need folks from @paketo-buildpacks/tooling-maintainers and the Paketo buildpacks that use packit to chime in on this RFC.

I guess that is the main point. Together with the styling (e.g. italics), it makes the log easier to read.

The tricky part here is that this mostly amounts to personal preference. I'm sure there are folks that like the packit output style better. If we want this to work, and not turn into a bike shedding exercise, my thought is that we need to focus on the problems solved by the two formats and see if we can agree on a single format that addresses the problems.

We know that color output is one issue. The colors look nice to some people, others may not want it. Some CI systems also don't handle color output well, so you see the actual terminal characters and not the color. I think this is easy to solve though, we can set an env variable that disables color output. I think NO_COLOR is standard for this, but if someone knows for sure please correct me. The format would be the same, but we just turn off all the color/special formatting.

I'd invite others to chime in with problems that are solved in their respective logging format (i.e. why is the format the way it is) so that we can focus on those and make sure the result of this RFC covers those issues.

Other issues that come to mind:

  1. In libpak, we use color/bold primarily to indicate warning messages. Things the user should know about. We should probably agree on a standard format for this. We usually prefix the line with WARNING: that way even without color/special formatting it's clear there is something important.

  2. In libpak, we use color/bold (in addition to indentation) to indicate different buildpacks. This breaks down the output so you can scan and quickly find the buildpack you want. We also have a specific format for the buildpack's title line, so it's easily identifiable, Paketo Buildpack for Amazon Corretto 8.5.2 (buildpack name buildpack version).

  3. In libpak, we use color/bold (in addition to indentation) to indicate different sections of the buildpack. This helps to break down the sections of the buildpack so if there's a problem, you can more quickly identify what part failed.

  4. In libpak, We use two spaces to indent the next section of logs. I don't think the number of spaces really matters, just including this for reference.

  5. In libpak, we use a table to output the configuration settings. There are columns for the setting name, detected value, and the setting description. We show all of the possible configurations, not just what the user has set. This makes it very clear to users what settings are available and how they are applied when the buildpack ran.

@c0d1ngm0nk3y
Copy link
Contributor Author

c0d1ngm0nk3y commented Sep 19, 2024

Instead of rewriting the same code, we could add a packit.bard with has the same interface and internally uses libpak.bard. So any packit based buildpack could use this logging as well, but it should not confuse the use since libpak is only an internal dependency.

Thoughts?

I could try this for nodejs @paketo-buildpacks/nodejs-maintainers

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