Skip to content

Support Warning header aggregation and reporting in crane#1604

Merged
imjasonh merged 5 commits intogoogle:mainfrom
imjasonh:warn-real
Mar 24, 2023
Merged

Support Warning header aggregation and reporting in crane#1604
imjasonh merged 5 commits intogoogle:mainfrom
imjasonh:warn-real

Conversation

@imjasonh
Copy link
Copy Markdown
Contributor

@imjasonh imjasonh commented Mar 18, 2023 •
edited
Loading

opencontainers/distribution-spec#393 seems to be on a good path, so I figured I'd put in a better implementation of warning collection/reporting in ggcr.

  • adds an internal transport that collects and reports warnings from registry requests
  • adds registry.WithWarning to pkg/registry that probabilistically reports the given warning (WithWarning(0.1, "hi") warns "hi" 10% of the time)

This also updates cmd/registry to add demo warnings some % of the time; I'll probably revert this before merging, this is just for demo purposes. Speaking of demo:

$ go run ./cmd/registry &
$ go run ./cmd/crane cp alpine localhost:1338/alpine
...
[WARNING]: 60% of the time, it works every time.
[WARNING]: This registry is cool.

Logic for parsing the warning message out of the header is pretty janky, and could use some tests.

Clients that use go-containerregistry won't suddenly start reporting warnings without changes. They'll need to inject a transport like crane does -- if we think crane's approach is generally useful we can make it available in pkg/crane itself.

@imjasonh imjasonh requested a review from jonjohnsonjr March 18, 2023 20:43
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 18, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 72.74%. Comparing base (0f2db49) to head (49501ff).
Report is 110 commits behind head on main.

Files with missing lines Patch % Lines
pkg/registry/registry.go 0.00% 11 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1604      +/-   ##
==========================================
- Coverage   72.80%   72.74%   -0.07%     
==========================================
  Files         118      118              
  Lines        9440     9452      +12     
==========================================
+ Hits         6873     6876       +3     
- Misses       1869     1878       +9     
  Partials      698      698

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imjasonh imjasonh changed the title Support Warning header aggregation and reporting Support Warning header aggregation and reporting in crane Mar 18, 2023
liggitt
liggitt reviewed Mar 19, 2023
View reviewed changes
@jonjohnsonjr
Copy link
Copy Markdown
Collaborator

jonjohnsonjr commented Mar 20, 2023

I think there are a couple ways we could go with this that will be slightly better than adding WithWarnCollector to crane and remote.

We already have logs.Warn (used here in this PR currently). We could just have warnings be written to logs.Warn where we set logs.Warn to do warncollectory things.

There's also remote.WithProgress that we currently use to emit very specific kinds of updates. We could do something similar (or even use the same option) to send clients more kinds of events, e.g. these Warnings.

I really dislike the logs package -- it would probably be better if we were just emitting events and having consumers do things with them...

There's also the slog stuff that has its own Warn level, which maybe we could replace the logs package with 🤔

How do you feel about this not being a public API, and instead we have crane pass in a custom transport that does all these warncollectory things until we have a happier public interface?

liggitt
liggitt reviewed Mar 23, 2023
View reviewed changes
@imjasonh
Copy link
Copy Markdown
Contributor Author

imjasonh commented Mar 23, 2023

How do you feel about this not being a public API, and instead we have crane pass in a custom transport that does all these warncollectory things until we have a happier public interface?

I'm fine with that. I'll get something along these lines up shortly.

jonjohnsonjr
jonjohnsonjr reviewed Mar 24, 2023
View reviewed changes
@imjasonh imjasonh enabled auto-merge (squash) March 24, 2023 17:40
@imjasonh imjasonh merged commit a34235c into google:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonjohnsonjr jonjohnsonjr jonjohnsonjr approved these changes

+1 more reviewer

@liggitt liggitt liggitt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@imjasonh @codecov-commenter @jonjohnsonjr @liggitt